-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Preserve scalar types when using the replacement filter #4494
Preserve scalar types when using the replacement filter #4494
Conversation
Welcome @lack! |
Hi @lack. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/ok-to-test |
- select: | ||
kind: Pod | ||
fieldPaths: | ||
- spec.containers.0.ports.0.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.
Question, what happens in the scenario that this path does not yet exist and we have to create it? e.g.
replacements:
- source:
kind: ConfigMap
name: config
fieldPath: data.PORT
targets:
- select:
kind: Pod
fieldPaths:
- spec.containers.0.ports.0.port
options:
create: true
Would you mind adding a test for this case? No need to fix the behavior in this PR if it does something undesirable - but it would be good to just have the test as a record of the behavior.
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.
Good idea!
Current behavior would be to create it with the source type (since Kustomize doesn't know what the target type is if it didn't already parse it)
Will add!
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.
Added. Though it did expose another problem in the replacement code: Apparently the "create" option doesn't deal well with appending to arrays or creating intermediate maps or arrays.
I'll log separate issues about those.
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.
Turns out intermediate maps are fine. Intermediate arrays are complicated. Following up on that in #4507
e76b84e
to
c00ed07
Compare
Maybe I'll add an explicit test for boolean conversion as well as int? |
Yes please! Thank you. Once that is done I'll do another pass and we can get this PR in. |
Erasing the scalar type tag leads to unfortunate circumstances, in that the resulting yaml code is valid yaml, but will not meet the object spec. For example, using the replacement transformer to take a port number as a string from a ConfigMap and set a Pod port would previously end up with: - containerPort: "8080" when the spec requires that this is not a string: - containerPort: 8080 Added unit tests for conversion to and from integers and booleans, plus creation from string and creation from integer. The creation behavior needs some refinement in a future PR. Signed-off-by: Jim Ramsay <i.am@jimramsay.com>
c00ed07
to
cb80659
Compare
Okay! Latest change adds unit tests for conversion to/from boolean as well. |
@natasha41575 ^^ Ready to go, I think! |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lack, 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 |
Fixes #4479 |
Erasing the scalar type tag leads to unfortunate circumstances, in that
the resulting yaml code is valid yaml, but will not meet the object
spec.
For example, using the replacement transformer to take a port number as
a string from a ConfigMap and set a Pod port would previously end up
with:
when the spec requires that this is not a string:
Signed-off-by: Jim Ramsay i.am@jimramsay.com