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

Standardize labels with k8s-app #3154

Closed
wants to merge 1 commit into from
Closed

Standardize labels with k8s-app #3154

wants to merge 1 commit into from

Conversation

hswong3i
Copy link
Contributor

We are using k8s-app as label in kubespray

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 22, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: riverzhang

If they are not already assigned, you can assign the PR to them by writing /assign @riverzhang in a comment when ready.

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

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 22, 2018
@ant31
Copy link
Contributor

ant31 commented Aug 22, 2018

if you go on this path you should use the recommended labels:
https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/

Key Description Example Type
app.kubernetes.io/name The name of the application mysql string
app.kubernetes.io/instance A unique name identifying the instance of an application wordpress-abcxzy string
app.kubernetes.io/version The current version of the application (e.g., a semantic version, revision hash, etc.) 5.7.21 string
app.kubernetes.io/component The component within the architecture database string
app.kubernetes.io/part-of The name of a higher level application this one is part of wordpress string
app.kubernetes.io/managed-by The tool being used to manage the operation of an application helm string

ant31
ant31 reviewed Aug 22, 2018
View reviewed changes
roles/kubernetes-apps/ansible/templates/netchecker-agent-ds.yml.j2
@@ -2,15 +2,15 @@ apiVersion: extensions/v1beta1
kind: DaemonSet
metadata:
labels:
app: netchecker-agent
k8s-app: netchecker-agent
Copy link
Contributor

Choose a reason for hiding this comment

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

app.kubernetes.io/name

ant31
ant31 reviewed Aug 22, 2018
View reviewed changes
...etes-apps/external_provisioner/cephfs_provisioner/templates/deploy-cephfs-provisioner.yml.j2
@@ -5,18 +5,18 @@ metadata:
name: cephfs-provisioner-{{ cephfs_provisioner_image_tag }}
namespace: {{ cephfs_provisioner_namespace }}
labels:
app: cephfs-provisioner
k8s-app: cephfs-provisioner
version: {{ cephfs_provisioner_image_tag }}
Copy link
Contributor

Choose a reason for hiding this comment

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

app.kubernetes.io/name
app.kubernetes.io/version

ant31
ant31 reviewed Aug 22, 2018
View reviewed changes
roles/kubernetes-apps/ingress_controller/cert_manager/templates/clusterrole-cert-manager.yml.j2
@@ -4,7 +4,7 @@ kind: ClusterRole
metadata:
name: cert-manager
labels:
app: cert-manager
k8s-app: cert-manager
chart: cert-manager-v0.4.1
release: cert-manager
heritage: Tiller
Copy link
Contributor

Choose a reason for hiding this comment

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

why is there 'tiller' label when it's not deployed via tiller ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same note for chart and release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy & Past from official manifest example for simplify integration overhead (i.e. lesser result during diff):

ant31
ant31 requested changes Aug 22, 2018
View reviewed changes
Copy link
Contributor

@ant31 ant31 left a comment

Choose a reason for hiding this comment

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

use 'official' labels

@hswong3i
Copy link
Contributor Author

I am just trying to “standardize” the use of k8s-app or app in kubespray, but not going to “choose” which label should be use or official...

Could I simply have an answer that: use k8s-app or app in kubespray?

jjungnickel
jjungnickel suggested changes Aug 22, 2018
View reviewed changes
Copy link
Contributor

@jjungnickel jjungnickel left a comment

Choose a reason for hiding this comment

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

I'm hesitant to accept changes to labels that have been there for a long time without a clear migration path. This could break customer tooling that expects some labels to be there.

Maybe one could standardize for the time being on having app, k8s-app and the official ones, slowly deprecating both app and k8s-app?

@hswong3i
Copy link
Contributor Author

For addons that I keep on migrate and sync to kubespray, e.g. weave, ingress-nginx, cert-manager and cephfs-provisioner, they are all using “app” in their “official” manifests example, so I can’t see why not keep them as-is “app” and so simplify the migration path.

BTW, I also respect the kubespray’s style, so duing migration I keep try to “change” them to “k8s-app”, but this really annoying :-(

@ant31
Copy link
Contributor

ant31 commented Aug 22, 2018 •
edited
Loading

@hswong3i what is the repo of the addons you maintain?

@hswong3i
Copy link
Contributor Author

https://github.com/kubernetes/ingress-nginx/tree/master/deploy

https://github.com/jetstack/cert-manager/tree/master/contrib/manifests/cert-manager

https://github.com/kubernetes-incubator/external-storage/tree/master/ceph/cephfs/deploy

For weave manifests generate by following their official release installation procedure and so download from weave.net

@ant31
Copy link
Contributor

ant31 commented Aug 22, 2018

@hswong3i
kubernetes/ingress-nginx#2970

@Atoms Atoms changed the title Standardize labals with k8s-app Standardize labels with k8s-app Aug 22, 2018
@Atoms
Copy link
Member

Atoms commented Aug 24, 2018

I agree, that we need to change to k8s proposed naming so all can relay, for sake of migration we can leave both for now, and just add Deprecation note on labels on next release

@hswong3i
Copy link
Contributor Author

hswong3i commented Sep 5, 2018

@ant31 it is not such simple for fixing upstream... e.g. kubernetes/ingress-nginx#2970 need follow up with kubernetes/ingress-nginx#3035

@ant31
Copy link
Contributor

ant31 commented Sep 5, 2018

@ant31 it is not such simple for fixing upstream

yep, thanks. I still have to open issues on other upstream projects.

@hswong3i hswong3i closed this Nov 16, 2018
@hswong3i hswong3i deleted the k8s-app branch November 16, 2018 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@desaintmartin desaintmartin desaintmartin left review comments

@ant31 ant31 ant31 requested changes

@jjungnickel jjungnickel jjungnickel requested changes

@chapsuk chapsuk Awaiting requested review from chapsuk

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hswong3i @k8s-ci-robot @ant31 @Atoms @desaintmartin @jjungnickel