-
Notifications
You must be signed in to change notification settings - Fork 578
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
Fix the backport of #4512 into release-2.2 branch #4590
Fix the backport of #4512 into release-2.2 branch #4590
Conversation
13c4512
to
b44cfd3
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: richardcase 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 |
@richardcase @cnmcavoy - how does this work now? Is there a new release of 2.2? aka 2.2.5? Thanks for making this fix - I'd been meaning to do it. I just yesterday double checked the behavior of Same thing said differently, based on my (local) testing yesterday, #4512 should not be added to 2.2.4 release notes as the issue isn't fixed there. This pr should fix it in the next release of 2.2. |
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.
looks good to me.
I tested (tilt based test setup) that someone with a cluster with CAPA 2.2.4 can upgrade to this patch (future 2.2.5) and it works properly. I was concerned that the prevAnnotation annotation might not push tag updates through after upgrades but it does.
EDIT: for more context, prevAnnotations
isn't even used when updating tags (which is good - prevents issues with an annotation's state being wrong - but it does mean more API tag requests - on every reconcile i believe); prevAnnotations
is used when deciding when to update the annotation which says which tags were last applied.
@philjb - once this merges we can thinking about doing a v2.2.5 release. |
We just need some to review and provide the lgtm |
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
What type of PR is this?
/kind regression
What this PR does / why we need it: Regression from main branch, backport of #4512 into release-2.2 failed and introduced errors and missed adding a unit test. See conversation on slack by @philjb : https://kubernetes.slack.com/archives/CD6U2V71N/p1697658532510839
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 #4511
Special notes for your reviewer:
Checklist:
Release note: