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 bug with migrating kyaml reader path, index, and id annotations #4227
fix bug with migrating kyaml reader path, index, and id annotations #4227
Conversation
97115d9
to
f38d44b
Compare
f38d44b
to
3feb55d
Compare
f665898
to
28f1661
Compare
43fd08a
to
ab32c94
Compare
ab32c94
to
00335de
Compare
@@ -299,7 +299,13 @@ g: | |||
`, | |||
}, | |||
|
|||
expectedOutput: `a: b #first | |||
expectedOutput: `e: f |
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 reordering is undoing the reordering done by the original PR https://github.com/kubernetes-sigs/kustomize/pull/4190/files
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 this considered as a breaking change?
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 wasn't considered a breaking change in #4190 when the reordering occurred, so I don't think it would be considered a breaking change to reverse 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.
Why do these annotations cause an ordering change, and under what circumstances? Any reordering that surfaces in Kustomize itself should be considered breaking. But judging by this being the only test where it surfaces, I take it only direct kyaml use is affected?
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.
Any reordering that surfaces in Kustomize itself should be considered breaking.
I agree and this change only surfaced directly in kyaml. The reordering occurred in the test case where the nodes were being ordered - specifically in this case, some of the resources had the path/index annotations while this one did not. My guess (though I haven't done any investigation to back this up) is that the bug (introduced in #4190) which caused the legacy annotations to be sometimes overwritten or ignored caused the reordering, and this PR fixed it along with its other fixes.
/hold Want to wait for some other manual tests to pass before merging this |
/unhold |
metadata: | ||
name: foobar | ||
annotations: | ||
internal.config.kubernetes.io/path: 'a/b.yaml' |
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.
When both exist and don't match, should we error it out somewhere?
If we are handling it in another place, can you please help me recall 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.
This is handled in reconcileInternalAnnotations
, which detects when both exist and don't match and reconciles them accordingly. However this is only checked after running a function to account for functions that mutate one but not the other.
If the input resource to an orchestrator has both set but they differ, the orchestrator will prefer the internal annotation over the legacy one. Do you believe it should throw an error instead?
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 should error when the input to the function has both new and legacy annotations, the function modify both annotations and they mismatch. In this case, we can't infer the user's intend, we should error. It should be fine in other cases as long as we can infer user's intend.
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.
sounds good. I will update it to throw an error instead.
@@ -299,7 +299,13 @@ g: | |||
`, | |||
}, | |||
|
|||
expectedOutput: `a: b #first | |||
expectedOutput: `e: f |
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 this considered as a breaking change?
2c637fb
to
67d8499
Compare
67d8499
to
55ac9ca
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mengqiy, natasha41575 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 |
In writing an e2e test for kpt, I discovered a couple issues with the migration of the path/index/id annotation code I'd written earlier. The legacy annotations were being overwritten by the internal ones after being processed by functions. This PR fixes these bugs.
copyAnnotations
should copy the annotations from one annotation (internal or legacy) to the other only if the other is empty. That way, if a function changes one but not the other we still have both andkio.reconcileInternalAnnotations
will detect that they are different and reconcile them. Additionally, I added a check to detect if either the legacy or internal index annotation is set, in which case it should be copied to the other and not overwritten by the reader.I've also added some unit tests to verify that it now behaves correctly.