Skip to content
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

No Test for Configuration more specific than default #4811

Closed
annasong20 opened this issue Sep 28, 2022 · 4 comments
Closed

No Test for Configuration more specific than default #4811

annasong20 opened this issue Sep 28, 2022 · 4 comments
Assignees
Labels
kind/test-coverage Categorizes issue or PR as related to a gap in or problem with our test coverage. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@annasong20
Copy link
Contributor

annasong20 commented Sep 28, 2022

Is your feature request related to a problem? Please describe.

We document the functionality of the kustomize field configurations, but we lack test coverage and comments for the following case:

According to the documentation, in such a case, the more specific user-specified configuration should replace the more generic default and the transformer should not be applied to field specs that fall under the latter, but not the first.

See issue #4722 for reference.

FYI: We believe the code that implements the intended behavior is in accumulateTarget(). This method call appends the user-specified config to the list of configs, but then moves it to the beginning via sort. Thus, when this list of configs is merged with an empty list, the latter, more generic default config is marked as redundant and not included in the final configs here.

Describe the solution you'd like

We should have at least 1 test covering such a case. As stated above, because the code that implements this behavior is located here, the solution will probably be an integration test under kusttarget_test.go or api/krusty that follows a similar setup in #4722.

A few comments, say above MergeConfig(), the sortFields() call, and the sortFields() definition, to explain their purpose would also be very helpful.

@annasong20 annasong20 added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 28, 2022
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Sep 28, 2022
@KnVerey KnVerey added kind/test-coverage Categorizes issue or PR as related to a gap in or problem with our test coverage. and removed kind/feature Categorizes issue or PR as related to a new feature. labels Sep 28, 2022
@KnVerey
Copy link
Contributor

KnVerey commented Sep 28, 2022

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 28, 2022
@ChristopherFry
Copy link
Contributor

/assign

@annasong20
Copy link
Contributor Author

/close

Addressed by #4882.

@k8s-ci-robot
Copy link
Contributor

@annasong20: Closing this issue.

In response to this:

/close

Addressed by #4882.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/test-coverage Categorizes issue or PR as related to a gap in or problem with our test coverage. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

4 participants