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.

Sign up for GitHub

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

Jump to bottom

add support for labels and annotations in deployment template options #1812

Closed
wants to merge 1 commit into from
Closed

add support for labels and annotations in deployment template options #1812

wants to merge 1 commit into from

Conversation

charlesmorin
Copy link
Contributor

Changes

  • Add support for adding labels and annotations in Deployment pod template (i.e. spec.template.metatada.labels and spec.template.metatada.annotations).
  • Add docs for new fields
  • Fix typos in existing docs

Sample

apiVersion: apps/v1
kind: Deployment
metadata:
  name: tekton-pipelines-controller
  namespace: tekton-pipelines
  creationTimestamp: null
  labels:
    app.kubernetes.io/name: controller
    controlled-by-options: "true"
  annotations:
    hpa-enabled: "false"
status: {}
spec:
  strategy: {}
  replicas: 4
  selector:
    matchLabels:
      app.kubernetes.io/name: controller
  template:
    metadata:
      creationTimestamp: null
      labels:
        app.kubernetes.io/name: controller
        operator.tekton.dev/deployment-spec-applied-hash: 92745dd0d03f316d534b70284c114dd5
        owner: "some-team"             <-------
      annotations:
        prometheus.io/scrape: "true"   <-------
        prometheus.io/port: "9090"     <-------
    spec:
      affinity:
        nodeAffinity:
        ...

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Release Notes

Add support for adding labels and annotations on Deployment's podTemplate.

@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Oct 29, 2023
@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign vdemeester after the PR has been reviewed.
You can assign the PR to them by writing /assign @vdemeester in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@tekton-robot tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 29, 2023
@tekton-robot
Copy link
Contributor

Hi @charlesmorin. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 29, 2023
charlesmorin
charlesmorin commented Oct 30, 2023
View reviewed changes
pkg/reconciler/common/transformer_additional_options.go
@@ -135,6 +135,60 @@ func (ot *OptionsTransformer) updateAnnotations(u *unstructured.Unstructured, an
return nil
}

func (ot *OptionsTransformer) updatePodTemplateLabels(u *unstructured.Unstructured, labels map[string]string) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to go with NestedMap, RemoveNestedField and SetNestedMap here given there's no method for pod templates in https://github.com/tektoncd/operator/blob/main/vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go

@jkandasa
Copy link
Member

jkandasa commented Nov 2, 2023

/ok-to-test

@tekton-robot tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 2, 2023
@tekton-robot
Copy link
Contributor

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/common/transformer_additional_options.go 89.9% 87.7% -2.2

@charlesmorin charlesmorin force-pushed the podtemplate-label-annotations-options branch from 84f31e7 to a4d5ebe Compare November 3, 2023 12:46
@tekton-robot tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 3, 2023
@tekton-robot
Copy link
Contributor

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/common/transformer_additional_options.go 89.9% 87.7% -2.2

@charlesmorin charlesmorin force-pushed the podtemplate-label-annotations-options branch from a4d5ebe to 194b4b2 Compare November 3, 2023 12:52
@tekton-robot
Copy link
Contributor

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/common/transformer_additional_options.go 89.9% 87.7% -2.2

@charlesmorin charlesmorin force-pushed the podtemplate-label-annotations-options branch from 194b4b2 to 22e5d84 Compare November 3, 2023 12:58
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 3, 2023
@tekton-robot
Copy link
Contributor

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/common/transformer_additional_options.go 89.9% 87.7% -2.2

@charlesmorin
Copy link
Contributor Author

/retest

@charlesmorin charlesmorin force-pushed the podtemplate-label-annotations-options branch from 22e5d84 to c77d4e3 Compare November 3, 2023 15:50
@tekton-robot
Copy link
Contributor

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/common/transformer_additional_options.go 89.9% 87.0% -2.9

jkandasa
jkandasa reviewed Nov 6, 2023
View reviewed changes
pkg/reconciler/common/transformer_additional_options.go
actualLabels[labelKey] = labelValue
}

if actualLabels == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@charlesmorin actualLabels never be nil right? You have already initialized with empty map, if it is nil

pkg/reconciler/common/transformer_additional_options.go
actualAnnotations[annotationKey] = annotationValue
}

if actualAnnotations == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@charlesmorin actualAnnotations never be nil right? You have already initialized with empty map, if it is nil

docs/TektonConfig.md
labels:
custom-label: "foo"
annotations:
custom-annotation: "foo"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add the same in the deployments and statefulSets documentation?

@tekton-robot
Copy link
Contributor

@charlesmorin: PR needs rebase.

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.

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2023
@jkandasa jkandasa mentioned this pull request Jan 19, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkandasa jkandasa jkandasa left review comments

@piyush-garg piyush-garg Awaiting requested review from piyush-garg

@PuneetPunamiya PuneetPunamiya Awaiting requested review from PuneetPunamiya

Assignees
No one assigned
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@charlesmorin @tekton-robot @jkandasa