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
Bug 1973065: Preserve user annotations while editing an app #9315
Bug 1973065: Preserve user annotations while editing an app #9315
Conversation
@divyanshiGupta: This pull request references Bugzilla bug 1973065, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Bugzilla (gamore@redhat.com), skipping review request. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
8951776
to
9ce7861
Compare
/cc: @andrewballantyne |
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.
@rohitkrai03 / @christianvogt Did you guys wanna chime in here?
'image.openshift.io/triggers', | ||
'alpha.image.policy.openshift.io/resolve-names', | ||
'jarFileName', | ||
]; |
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.
So we dropped 'kubectl.kubernetes.io/last-applied-configuration'
as well in the Pipeline world.
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.
You meant that this annotation should be added here too?
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.
Yes that is what I am suggesting... Hmmm, this flow is only for editing or also for some creating? I don't think you can create a resource with 'kubectl.kubernetes.io/last-applied-configuration'
. I think the Devfile flow is a bit different during it's create resources that are crafted by the backend. If we copy two resources to create, I am 90% sure this will block the resource creation. At least it did for Pipelines: https://issues.redhat.com/browse/ODC-4917
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.
Hmm I think we might want to remove it as I saw this annotation in other resources as well though we dont add this specifically from our add flows but it might interfere with updating the resources like it probably did with PipelineRuns
though I am not 100% sure.
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.
I think updates are different than creates. Updates I wanna say are good with this annotation being echoed because it's an update. But I don't think you can create a resource with it... I should try to test once I get a moment today to make sure my representation of the world is actually correct lol.
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.
We are not creating any resource with this annotation from add flows and I dont think that we have a similar case as PipelineRuns
here or I am not aware of it.
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.
Hmmm, okay, let's ignore this thread -- I'm having issues reproducing it but my cluster went down as I was playing with various K8s / CRD kinds. Clearly this doesn't seem to fit the issue at hand, at least not directly.
Edit: Ah you added it anyways. Appreciate that, shouldn't be a problem having it there.
9ce7861
to
e1e0d2b
Compare
/assign @rohitkrai03 |
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.
Tested, lgtm. Definitely would help seeing if there are any other annotations that we want to be weary of... But this should be a good one to back port.
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne, divyanshiGupta, rohitkrai03 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 |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
@divyanshiGupta: All pull requests linked via external trackers have merged: Bugzilla bug 1973065 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cherry-pick release-4.8 |
@andrewballantyne: new pull request created: #9381 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cherry-pick release-4.7 |
@divyanshiGupta: new pull request created: #9714 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Fixes: https://issues.redhat.com/browse/OCPBUGSM-31048
Analysis/Root Cause:
Annotations cannot be edited directly via edit flows like labels but the default annotations values are updated if one or more form field values are modified. In order to keep the default annotations values up-to date old annotations were not being added and in turn the user annotations were also being lost.
Solution:
Only update the default annotations and keep the user annotations as is.
Screen-recording:
Screen.Recording.2021-06-22.at.8.26.37.PM.mov
Test coverage:
/kind bug