-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support pre-sync annotations #1433
base: main
Are you sure you want to change the base?
Support pre-sync annotations #1433
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
pkg/parse/root.go
Outdated
return status.APIServerError(err, "failed to get RootSync for parser") | ||
} | ||
existing := rs.DeepCopy() | ||
core.SetAnnotation(rs, metadata.SourceCommitAnnotationKey, commit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest checking the existing annotations to avoid unnecessary updates if they're the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added check. Also added similar check in resetSourceAnnotations when removing the annotations when sourceType switched or set to Git.
8832a4f
to
fc6cf85
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add context to the commit message and PR description
pkg/metadata/annotations.go
Outdated
|
||
// ImageURLAnnotationKey is the annotation key applied to R*Sync objects | ||
// when pre-sync is either disabled or successfully completed. | ||
// This annotation stores the Git commit hash or image digest. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is misleading, it looks like this never maps to a Git commit hash
. It's currently only set if the sourceType != git
, i.e. for oci or helm. Is this behavior intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just modified the comment to align with behavior.
@@ -261,6 +261,38 @@ func setSourceStatusFields(source *v1beta1.SourceStatus, newStatus *SourceStatus | |||
source.LastUpdate = newStatus.LastUpdate | |||
} | |||
|
|||
func (p *root) setSourceAnnotations(ctx context.Context, commit string) error { | |||
rs := &v1beta1.RootSync{} | |||
if err := p.Client.Get(ctx, rootsync.ObjectKey(p.SyncName), rs); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the get necessary? Seems like this patch does not require a get
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It compares the annotations with the new values to avoid unnecessary patching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, instead of merging from existing object, we can patch directly with the raw patch data, client.RawPatch
. With that, we don't need to GET and check for changes.
err = p.setSourceAnnotations(ctx, gs.Commit) | ||
} | ||
if err != nil { | ||
gs.Errs = status.Append(gs.Errs, status.SourceError.Wrap(err).Build()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no test cases which cover this scenario
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, setting or removing annotations should always succeed since the end-to-end test involving the webhook check has not been implemented yet. In the future, when the webhook service that verifies image signatures is in place, we will be able to simulate error scenarios where patching (setting annotations) fails, and then validate the source errors accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO that functionality is fundamental to this feature and the feature should not be submitted without test coverage for it. What's the motivation for submitting this feature before validating the webhook user journey?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair, I could keep this open until the commit for the full user journey is ready. The original intention was to keep the steps small and make this change only adds the annotations, maybe I can leave the part of returning the SourceError out from this PR for now?
fc6cf85
to
78d56dd
Compare
pkg/parse/run.go
Outdated
if gs.Errs == nil { | ||
var err error | ||
if opts.SourceType == "git" { | ||
err = p.resetSourceAnnotations(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest merging resetSourceAnnotations
and setSourceAnnotations
.
The consolidated setSourceAnnotations
function should
- always set the
source-commit
annotation. - Do not set the
image-url
annotation for Git sources, while set it top.Options.SourceRepo
for OCI and Helm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future-proofing, let's rename image-url
to source-url
, but still keep it unset for the Git source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with the latest patch, the source-commit is no longer removed when switching back to git. Modified the tests to reflect that.
dcfca04
to
62ce848
Compare
62ce848
to
dd88b20
Compare
dd88b20
to
c11e29b
Compare
No description provided.