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

[Helm] Add labels to resources #6992

Merged
merged 5 commits into from
Nov 19, 2021

Conversation

hamza3202
Copy link
Contributor

@hamza3202 hamza3202 commented Mar 25, 2021

What this PR does / why we need it:

This PR allows the user to add labels to resources by providing helm values. It is important to label resources based on user's organizational policies.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

How Has This Been Tested?

  • helm templating works with or without providing additional labels and provided labels are added to resources.

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 25, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @hamza3202. Thanks for your PR.

I'm waiting for a kubernetes 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.

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 25, 2021
@hamza3202
Copy link
Contributor Author

/assign @ChiefAlexander

@hamza3202
Copy link
Contributor Author

/assign @ElvinEfendi

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 26, 2021
@hamza3202 hamza3202 changed the title [Helm] Add labels to RBAC resources [Helm] Add labels to resources Mar 26, 2021
@ChiefAlexander
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-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 Mar 31, 2021
@hamza3202
Copy link
Contributor Author

/test

@k8s-ci-robot
Copy link
Contributor

@hamza3202: The /test command needs one or more targets.
The following commands are available to trigger jobs:

  • /test pull-ingress-nginx-boilerplate
  • /test pull-ingress-nginx-codegen
  • /test pull-ingress-nginx-gofmt
  • /test pull-ingress-nginx-golint
  • /test pull-ingress-nginx-lualint
  • /test pull-ingress-nginx-chart-lint
  • /test pull-ingress-nginx-test-lua
  • /test pull-ingress-nginx-test
  • /test pull-ingress-nginx-e2e-1-16
  • /test pull-ingress-nginx-e2e-1-17
  • /test pull-ingress-nginx-e2e-1-18
  • /test pull-ingress-nginx-e2e-1-19
  • /test pull-ingress-nginx-e2e-helm-chart

Use /test all to run the following jobs:

  • pull-ingress-nginx-chart-lint
  • pull-ingress-nginx-e2e-helm-chart

In response to this:

/test

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.

@hamza3202
Copy link
Contributor Author

@ChiefAlexander Will it be okay to update version and changelog, once my other PR gets merged?

@hamza3202
Copy link
Contributor Author

@ChiefAlexander can you please merge this PR?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 1, 2021
@@ -5,6 +5,9 @@ metadata:
labels:
{{- include "ingress-nginx.labels" . | nindent 4 }}
app.kubernetes.io/component: default-backend
{{- with .Values.defaultBackend.labels }}
{{- toYaml . | nindent 4 }}
{{- end }}
Copy link

Choose a reason for hiding this comment

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

I'm curious if the above should also be included below at https://github.com/kubernetes/ingress-nginx/pull/6992/files#diff-b26819b1b2600e0b40ae78243b6b0efa9d34098f1594ac0838d23669bb93be18L23

Sure there are podLabels, but that would force the installer to duplicate some configuration if they should be the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jtslear
Copy link

jtslear commented May 10, 2021

I'm also curious if we want to send all controller labels to the pods as well? This would mean we'd need to add

    {{- with .Values.controller.labels }}
    {{- toYaml . | nindent 4 }}
    {{- end }

To:

@rikatz
Copy link
Contributor

rikatz commented Jun 6, 2021

/area helm

@k8s-ci-robot k8s-ci-robot added the area/helm Issues or PRs related to helm charts label Jun 6, 2021
@tao12345666333
Copy link
Member

need rebase

@rikatz rikatz changed the base branch from master to main July 16, 2021 13:09
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jul 16, 2021

@hamza3202: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-ingress-nginx-e2e-helm-chart fb981fe43d7d2e3e55e7a77856174fe2d0ba4df0 link /test pull-ingress-nginx-e2e-helm-chart

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@hamza3202 hamza3202 force-pushed the add-labels-rbac-resources branch from fb981fe to 6e72ce6 Compare November 18, 2021 17:11
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2021
Signed-off-by: Muhammad Hamza Zaib <hamzazaib3202@gmail.com>
Signed-off-by: Muhammad Hamza Zaib <hamzazaib3202@gmail.com>
@hamza3202 hamza3202 force-pushed the add-labels-rbac-resources branch from 6e72ce6 to c362d00 Compare November 18, 2021 17:14
@hamza3202
Copy link
Contributor Author

I got side tracked with other stuff. Please review this now.

@iamNoah1
Copy link
Contributor

/lgtm

thanks

@iamNoah1
Copy link
Contributor

/lifecycle active

@k8s-ci-robot k8s-ci-robot added lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Nov 19, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 19, 2021
@iamNoah1
Copy link
Contributor

/tide merge-method-squash

@iamNoah1
Copy link
Contributor

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 19, 2021
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

thanks for that, looks good to me and I did a local test and got the labels

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 19, 2021
@cpanato
Copy link
Member

cpanato commented Nov 19, 2021

/hold for @ChiefAlexander review as well

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 19, 2021
Copy link
Contributor

@ChiefAlexander ChiefAlexander left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ChiefAlexander, cpanato, hamza3202, iamNoah1

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:

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

@cpanato
Copy link
Member

cpanato commented Nov 19, 2021

thanks for the contribution
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 19, 2021
@k8s-ci-robot k8s-ci-robot merged commit 30c0d22 into kubernetes:main Nov 19, 2021
@hamza3202 hamza3202 deleted the add-labels-rbac-resources branch November 21, 2021 20:01
rchshld pushed a commit to joomcode/ingress-nginx that referenced this pull request May 19, 2023
* Add labels to RBAC resources

* Add labels to all resources

* Fix labels indentaton in patch jobs

* Add controller and default backend labels to pods

Signed-off-by: Muhammad Hamza Zaib <hamzazaib3202@gmail.com>

* Bump chart version and update changelog

Signed-off-by: Muhammad Hamza Zaib <hamzazaib3202@gmail.com>
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. area/helm Issues or PRs related to helm charts 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. lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants