kyaml/fn/framework ensures the annotation output format matches the input #4297
Conversation
…nput If the input only contains legacy format anntations (path, index, id), the output will be the same.
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
approved
|
/cc @natasha41575 @droot |
There was a problem hiding this comment.
I am not super familiar with the annotation machinery so would leave @natasha41575 and you to decide the final approval.
One note: overall the code for annotations seems to have become very complex. Not sure if there is way to reduce this complexity.
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
@mengqiy: This PR has multiple commits, and the default merge method is: merge. 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. |
|
/label tide/merge-method-squash |
k8s-ci-robot
added
the
tide/merge-method-squash
k8s-ci-robot
added
size/XL
|
@droot I changed it to use id as the key identify the objects. If we can't find id, we just skip it. kpt fn render and eval always set id, but |
mengqiy
force-pushed
the
anno
branch
2 times, most recently
from
fb99a28
to
f9e1628
Compare
| @@ -293,6 +293,8 @@ apiVersion: apps/v1 | |||
| kind: Deployment | |||
| metadata: | |||
| name: foo | |||
| annotations: | |||
| internal.config.k8s.io/annotations-migration-resource-id: '1' | |||
There was a problem hiding this comment.
This is part of the command test.
I'm not sure why it will need this change to pass.
k8s-ci-robot
removed
the
approved
|
You can ignore the presubmit failure |
|
/retest |
| @@ -63,6 +63,9 @@ xxx: | |||
| apiVersion: foo/v1 | |||
| kind: Bar | |||
| xxx: | |||
| metadata: | |||
| annotations: | |||
| internal.config.k8s.io/annotations-migration-resource-id: '0' | |||
There was a problem hiding this comment.
@KnVerey this is something we should discuss before the next release of the api module to decide if we are okay with the change.
What's happening is to migrate the annotations, kyaml/kio adds a new annotation to help keep track of the migration, then runs the filter, then removes the annotation. In the case that the filter errors, it includes in its error message the contents of the failing RNode. Unfortunately that means that this annotation gets included in the error message before we can remove it.
Because this is blocking kpt and doesn't show up in the output of kustomize (we've added the annotation to kustomize's BuildAnnotations so that it will get removed), I'm approving this PR to go in so that they can use an unreleased version of kyaml until we have a better solution/decision.
No changes need to be made if we decide we're okay with these annotations showing up in the output of api and cmd/config tests.
One option I was considering was to change the error message for the filters to just have the object's GVKNN, rather than the entire content of the RNode.
Another option is to clear the annotation in the filter functions before generating the error message, but we'd need to do that in several places, which doesn't seem ideal to me.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/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 |
k8s-ci-robot
added
the
approved
If the input only contains legacy format anntations (path, index, id), the
output will be the same.
ALLOW_MODULE_SPAN