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

Add recommended k8s labels #2501

Merged
merged 1 commit into from
May 14, 2020
Merged

Conversation

eddycharly
Copy link
Member

@eddycharly eddycharly commented Apr 28, 2020

This PR adds recommended k8s labels on resources.
Implementation for proposal #2497

Config: added labels and updated selectors for all resources in /config
Doc: added /docs/developers/resources-labelling.md

NOTE: this will introduce a breaking change when trying to update deployments as label selectors have changed.

@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 28, 2020
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 28, 2020
@eddycharly
Copy link
Member Author

/cc @vdemeester

@eddycharly
Copy link
Member Author

/test tekton-pipeline-unit-tests

1 similar comment
@eddycharly
Copy link
Member Author

/test tekton-pipeline-unit-tests

@eddycharly eddycharly force-pushed the k8s-labels branch 3 times, most recently from 78f359e to e6299f3 Compare April 28, 2020 11:42
@eddycharly eddycharly changed the title [WIP] Add recommended k8s labels Add recommended k8s labels Apr 28, 2020
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 28, 2020
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

+:100:
/meow

config/101-podsecuritypolicy.yaml Outdated Show resolved Hide resolved
@tekton-robot
Copy link
Collaborator

@vdemeester: cat image

In response to this:

+:100:
/meow

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 28, 2020
@eddycharly
Copy link
Member Author

/hold
I feel like there are probably some superfluous labels

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 28, 2020
@eddycharly eddycharly force-pushed the k8s-labels branch 3 times, most recently from ba83238 to 9e23236 Compare April 28, 2020 15:47
@eddycharly
Copy link
Member Author

/hold cancel
I removed app.kubernetes.io/name and app.kubernetes.io/version when they had no value.

@vdemeester how about using app.kubernetes.io/version everywhere instead of pipeline.tekton.dev/release, triggers.tekton.dev/release, dashboard.tekton.dev/release, etc ?

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 28, 2020
@eddycharly
Copy link
Member Author

/test tekton-pipeline-unit-tests

@vdemeester
Copy link
Member

/hold cancel
I removed app.kubernetes.io/name and app.kubernetes.io/version when they had no value.

@vdemeester how about using app.kubernetes.io/version everywhere instead of pipeline.tekton.dev/release, triggers.tekton.dev/release, dashboard.tekton.dev/release, etc ?

yeah we could, but we will need to keep those (at least for a while) there as tools depend on them.

@eddycharly
Copy link
Member Author

I know the dashboard depends on it, i am adding support for app.kubernetes.io/veersion there.
Any other tool ?

@vdemeester
Copy link
Member

I know the dashboard depends on it, i am adding support for app.kubernetes.io/veersion there.
Any other tool ?

  • tkn
  • OpenShift Pipeline
  • most likely a bunch of other tools that we do not control 😝

We should add the new one, mark the old one as "deprecated" (in release note) and decide on a time where it gets removed (if we want to remove it 😛 )

@eddycharly
Copy link
Member Author

/meow

@tekton-robot
Copy link
Collaborator

@eddycharly: cat image

In response to this:

/meow

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.

@eddycharly
Copy link
Member Author

@vdemeester do you want to include changes from #2508 in this PR ?

@vdemeester
Copy link
Member

@vdemeester do you want to include changes from #2508 in this PR ?

oh yeah we can 👍

@eddycharly
Copy link
Member Author

Done, i took the objectSelector approach, hope it's ok for you.

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 14, 2020
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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

@tekton-robot
Copy link
Collaborator

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

@eddycharly
Copy link
Member Author

/kind feature

@tekton-robot tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label May 14, 2020
@tekton-robot tekton-robot merged commit edc4539 into tektoncd:master May 14, 2020
@bobcatfish
Copy link
Collaborator

NOTE: this will introduce a breaking change when trying to update deployments as label selectors have changed.

In future can we put this in the "release notes" section of the pull request description? That is what I was looking for when I made the 0.13 releases and I missed this breaking change :(

bobcatfish added a commit to bobcatfish/pipeline that referenced this pull request Jun 15, 2020
While looking into tektoncd#2816
we realized that tektoncd#2501 had
indicated that this will require users to delete their Deployments but
since the note wasn't in a release notes block it was missed (by me!)

Adding this to the checklist might help contributors and reviewers be
more aware.

We also found that prow has a "release notes" plugin that might
check this for us on PRs if we turn it on:
https://github.com/kubernetes/test-infra/blob/128455dfd8003346afcdffa111d2298587f6fbf0/prow/plugins/releasenote/releasenote.go

Co-authored-by: Sharon Jerop Kipruto <[email protected]>
bobcatfish added a commit to bobcatfish/pipeline that referenced this pull request Jun 15, 2020
While looking into tektoncd#2816
we realized that tektoncd#2501 had
indicated that this will require users to delete their Deployments but
since the note wasn't in a release notes block it was missed (by me!)

Adding this to the checklist might help contributors and reviewers be
more aware.

We also found that prow has a "release notes" plugin that might
check this for us on PRs if we turn it on:
https://github.com/kubernetes/test-infra/blob/128455dfd8003346afcdffa111d2298587f6fbf0/prow/plugins/releasenote/releasenote.go

Co-authored-by: Sharon Jerop Kipruto <[email protected]>
tekton-robot pushed a commit that referenced this pull request Jun 16, 2020
While looking into #2816
we realized that #2501 had
indicated that this will require users to delete their Deployments but
since the note wasn't in a release notes block it was missed (by me!)

Adding this to the checklist might help contributors and reviewers be
more aware.

We also found that prow has a "release notes" plugin that might
check this for us on PRs if we turn it on:
https://github.com/kubernetes/test-infra/blob/128455dfd8003346afcdffa111d2298587f6fbf0/prow/plugins/releasenote/releasenote.go

Co-authored-by: Sharon Jerop Kipruto <[email protected]>
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. kind/feature Categorizes issue or PR as related to a new feature. lgtm 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.

4 participants