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

Remove local-only resources at the last minute #4895

Merged
merged 1 commit into from
Dec 1, 2022

Conversation

KnVerey
Copy link
Contributor

@KnVerey KnVerey commented Nov 30, 2022

This allows name references to local-only objects to be correctly resolved before the objects are removed from the set to be printed. The use case that made me think of this is the common Ingress situation from #4884, where there are several related values that are list items. If we can formally identify those values as names, then we can update them intelligently, even across multiple objects.

The side-effect is that local resources from a base will be exposed to overlays. Can you think of any reason that's undesirable? I was surprised it wasn't already the case, personally.

This allows name references to them to be correctly resolved
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 30, 2022
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 30, 2022
@KnVerey KnVerey added this to the v5.0.0 milestone Dec 1, 2022
@natasha41575
Copy link
Contributor

The side-effect is that local resources from a base will be exposed to overlays. Can you think of any reason that's undesirable? I was surprised it wasn't already the case, personally.

I don't see any reason why the local base resources shouldn't be exposed to overlays. I think this makes sense.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 1, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KnVerey, 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:
  • OWNERS [KnVerey,natasha41575]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 0f4f978 into kubernetes-sigs:master Dec 1, 2022
@KnVerey KnVerey deleted the remove-local-last branch December 1, 2022 23:15
@wyattanderson
Copy link

The side-effect is that local resources from a base will be exposed to overlays. Can you think of any reason that's undesirable? I was surprised it wasn't already the case, personally.

@KnVerey one side effect that we've now encountered is that a local-only configMapGenerator in a component used in multiple resources now has a name collision when those resources are combined into one kustomization. We're attempting to use replacements to change the registry domain of an image on a per-environment basis, and the only reason we use a configMapGenerator at all is because replacements can't reference constant values defined inline. Our approach works in 4.5.5, but breaks in 5.0.0 and above, I believe because of this change.

@mleklund
Copy link

I have the same issue with this a @wyattanderson. I am using replacements with a local-config confgiMapGenerator to differentiate blue/green deployments.

@annasong20
Copy link
Contributor

We want to gather more information before making a decision on this change.

We're attempting to use replacements to change the registry domain of an image on a per-environment basis, and the only reason we use a configMapGenerator at all is because replacements can't reference constant values defined inline.

@wyattanderson please feel free to open your own issue on this regression. Are there any workarounds for your use case? For example, you can

  • specify constant values with patches; though, it doesn't have the parsing abilities of replacements
  • move the reference to your Component into the overlay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants