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

[Feature] Set labels conditional spec selector #1064

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

kemv
Copy link

@kemv kemv commented Sep 13, 2023

The problem

I am facing a use-case where i would like to set common labels to all resources in my Kpt pipeline. To do this, i have tried using set-labels.

One of the common labels I want to set on all resources is app.kubernetes.io/version.

The problem is, set-labels also sets .spec.selector labels on deployments, and this field is immutable. This means, that when the version changes, all deployments have to be deleted and created again.

I propose some changes to have a switch to tell set-labels that it shouldn't set .spec.selectors. I have achieved this through the SetLabels kind already defined:

apiVersion: fn.kpt.dev/v1alpha1
kind: SetLabels
metadata:
  name: set-labels
  annotations:
    config.kubernetes.io/local-config: "true"
labels:
  app.kubernetes.io/version: "some-version"
options:
  setSelectorLabels: false # defaults to true

Considerations

I think that keeping backwards compatibility is very important. Therefore the option always defaults to true. Meaning if you change nothing, the functaionality doesn't change, even with this new feature. I haven't added this new functionality to the ConfigMap approach. This is because it would be harder to make this approach backwards compatible. The same with the imperative approach.

I have added a new e2e test for this functionality. I don't think this small functionality merits a new example page, but i have expanded the main README.md for the function.

@kemv
Copy link
Author

kemv commented Sep 14, 2023

@droot @mengqiy @sdowell @yuwenma Seems CODEOWNERS is broken? Or perhaps PRs are closed until migrated to CNCF ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant