-
Notifications
You must be signed in to change notification settings - Fork 430
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
refactor ASO label/annotation updates #3733
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #3733 +/- ##
=======================================
Coverage 54.12% 54.12%
=======================================
Files 187 187
Lines 18909 18917 +8
=======================================
+ Hits 10234 10239 +5
- Misses 8126 8128 +2
- Partials 549 550 +1
☔ View full report in Codecov by Sentry. |
if len(labels) > 0 { | ||
parameters.SetLabels(maps.Merge(parameters.GetLabels(), labels)) | ||
if len(labels) == 0 { | ||
labels = 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.
Why do we want to set labels
and annotations
back to nil? Would it make sense instead to call SetLabels()
and SetAnnotations()
only when existing == 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.
Why do we want to set
labels
andannotations
back to nil?
Because labels and annotations are both serialized with omitempty
, the existing
resource will never contain empty, non-nil values for either of those fields. This preemptively emulates omitempty
for the values of parameters
so when we calculate the diff just below these changes, we don't treat a non-nil and empty map as different from a nil one since they would both be serialized as nil.
Would it make sense instead to call
SetLabels()
andSetAnnotations()
only whenexisting == nil
?
Any update (when existing != nil
) may require labels and/or annotations to be updated (like when we notice a resource needs to be adopted), so I don't think that would work in all cases.
(squash reminder) |
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.
/assign @willie-yao @Jont828
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.
/lgtm
LGTM label has been added. Git tree hash: 2ea8eeb4a1842a99c8c97ab5f2da157063a2e71b
|
/lgtm |
done! |
/retest |
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR refactors how labels and annotations applied generically to ASO resources are constructed to enable
delete()
-ing existing labels or annotations which is planned for #3525. Otherwise this change should be functionally equivalent.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
TODOs:
Release note: