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 duplicate label behavior: commonLabels vs labels.includeSelectors #5436

Open
2 tasks done
ncapps opened this issue Nov 10, 2023 · 8 comments
Open
2 tasks done
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@ncapps
Copy link
Contributor

ncapps commented Nov 10, 2023

Eschewed features

  • This issue is not requesting templating, unstuctured edits, build-time side-effects from args or env vars, or any other eschewed feature.

What would you like to have added?

The commonLabels and labels fields with includeSelectors enabled produce the same output. It is preferable to have one method to add labels that are propagated to selectors and templates.

The sample kustomization files below would produce the same output.

# Add labels with labels and includeSelectors enabled
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

labels:
  - pairs:
      someName: someValue
      owner: alice
      app: bingo
    includeSelectors: true

resources:
- deploy.yaml
- service.yaml
# Add labels with commonLables
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

commonLabels:
  someName: someValue
  owner: alice
  app: bingo

resources:
- deploy.yaml
- service.yaml

Why is this needed?

We should remove duplicate behavior where possible to potentially simplify our kustomization API.

Can you accomplish the motivating task without this feature, and if so, how?

We can produce the same result with either commonLabels and labels fields.

What other solutions have you considered?

We can remove duplicate behavior with multiple approaches.

Option 1: Deprecate commonLabels

  • Deprecate the commonLabels field and exclusively use the labels field.
  • Suggest adding the annotations field with an includeTemplates option. This would mirror the labels field spec.

Option 2: Deprecate labels

  • Deprecate the labels field and add includeSelectors and includeTemplates options to the commonLabels field.
  • Suggest adding the includeTemplates option to the commonAnnotations field.

Anything else we should know?

This was identified when documenting labels and annotations behavior here: #5383

Feature ownership

  • I am interested in contributing this feature myself! 🎉
@ncapps ncapps added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 10, 2023
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Nov 10, 2023
@natasha41575
Copy link
Contributor

I agree that we should remove the duplicate behavior, but we need to be careful and adhere to the deprecation policy. Between the two options that you listed, I think it probably makes more sense to deprecate commonLabels as it would be fewer steps (just the deprecation), rather than the multiple steps of deprecating labels and then adding new functionality to commonLabels.

I think this has been discussed in the past, and rather unfortunately we cannot just remove a field from the kustomization definition as it is defined in the current v1beta1 version. We would have to include the removal as part of the definition of kustomization v1; then we would have to deprecate kustomization v1beta1 entirely. During the deprecation cycle (which I believe lasts at least 2 releases), we would have to support both kustomization v1 and kustomization v1beta1 before the eventual removal of kustomization v1beta1.

If you don't mind, I'd like to discuss this in the monthly kustomize bug scrub (there's one next week) so that we can make sure everyone is on the same page before we triage it.

@annasong20
Copy link
Contributor

/kind deprecation

@k8s-ci-robot k8s-ci-robot added the kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. label Nov 17, 2023
@annasong20
Copy link
Contributor

/remove-kind feature

@k8s-ci-robot k8s-ci-robot removed the kind/feature Categorizes issue or PR as related to a new feature. label Nov 17, 2023
@natasha41575 natasha41575 added this to To do in Kustomization v1 via automation Nov 22, 2023
@natasha41575
Copy link
Contributor

/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 Nov 22, 2023
@natasha41575
Copy link
Contributor

For now, we can add a deprecation warning to commonLabels letting users know that this will not be available in kustomization v1. We have a few other fields that we've deprecated this way, so we can follow the same model.

For long-term removal, we will have to move the kustomization v1 project forward, which has some nontrivial design & feature work.

/label good-first-issue
/help

@k8s-ci-robot
Copy link
Contributor

@natasha41575:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

  • Why are we solving this issue?
  • To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
  • Does this issue have zero to low barrier of entry?
  • How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

For now, we can add a deprecation warning to commonLabels letting users know that this will not be available in kustomization v1. We have a few other fields that we've deprecated this way, so we can follow the same model.

For long-term removal, we will have to move the kustomization v1 project forward, which has some nontrivial design & feature work.

/label good-first-issue
/help

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.

@k8s-ci-robot
Copy link
Contributor

@natasha41575: The label(s) /label good-first-issue cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

For now, we can add a deprecation warning to commonLabels letting users know that this will not be available in kustomization v1. We have a few other fields that we've deprecated this way, so we can follow the same model.

For long-term removal, we will have to move the kustomization v1 project forward, which has some nontrivial design & feature work.

/label good-first-issue
/help

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.

@k8s-ci-robot k8s-ci-robot added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Nov 22, 2023
@stormqueen1990
Copy link
Member

As noted in #5527 (comment), while labels offers feature parity with commonLabels, it removes some reusability aspects. commonLabels allows per-field transformer configuration to be added to a separate configuration file, which is helpful to avoid configuration duplication for situations when different kustomizations are in use. It might be worth considering an alternative mechanism for doing so before commonLabels gets entirely removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Development

No branches or pull requests

5 participants