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.
Suggest checking the existing annotations to avoid unnecessary updates if they're the same.
There was a problem hiding this comment.
Added check. Also added similar check in resetSourceAnnotations when removing the annotations when sourceType switched or set to Git.
tiffanny29631
force-pushed
the
oci-reconciler-change
branch
from
8832a4f
to
fc6cf85
Compare
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.
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.
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.
Is the get necessary? Seems like this patch does not require a get
There was a problem hiding this comment.
It compares the annotations with the new values to avoid unnecessary patching.
There was a problem hiding this comment.
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.
There are no test cases which cover this scenario
There was a problem hiding this comment.
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.
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.
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?
tiffanny29631
force-pushed
the
oci-reconciler-change
branch
from
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.
Suggest merging resetSourceAnnotations and setSourceAnnotations.
The consolidated setSourceAnnotations function should
- always set the
source-commitannotation. - Do not set the
image-urlannotation for Git sources, while set it top.Options.SourceRepofor OCI and Helm.
There was a problem hiding this comment.
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.
Done with the latest patch, the source-commit is no longer removed when switching back to git. Modified the tests to reflect that.
tiffanny29631
force-pushed
the
oci-reconciler-change
branch
4 times, most recently
from
cd2f486
to
dcfca04
Compare
tiffanny29631
force-pushed
the
oci-reconciler-change
branch
from
dcfca04
to
62ce848
Compare
No description provided.