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 app.kubernetes.io/version label #5373

Merged
merged 2 commits into from Apr 24, 2019

Conversation

Projects
None yet
4 participants
@luisdavim
Copy link
Contributor

commented Feb 27, 2019

What this PR does / why we need it:
app.kubernetes.io/version is a recommended label and I don't see any reason not to include it.

Special notes for your reviewer:
fixes #5369

@bacongobbler

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

@luisdavim

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Any updates on this?

@luisdavim luisdavim force-pushed the luisdavim:version_label branch from 9e6b2db to 2c6514b Mar 6, 2019

@helm-bot helm-bot added size/S size/M and removed size/S labels Mar 6, 2019

@luisdavim

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

I was waiting for this to be merged to open another PR to address #5372 but since it's taking a while to get this one through, I figured it would be better to address both issues in one go. So I've updated the PR, please let me know what you think.

@luisdavim luisdavim force-pushed the luisdavim:version_label branch from d16f802 to ce8f1f6 Mar 6, 2019

@helm-bot helm-bot added size/M and removed size/M labels Mar 6, 2019

@mattfarina
Copy link
Collaborator

left a comment

Thanks for PR. I'm happy to see better labeling in use. These are just some clarification nits.

@@ -10,6 +10,7 @@ metadata:
# The "release" convention makes it easy to tie a release to all of the
# Kubernetes resources that were created as part of that release.
app.kubernetes.io/instance: {{.Release.Name | quote }}
app.kubernetes.io/version: {{ .Chart.AppVersion }}

This comment has been minimized.

Copy link
@mattfarina

mattfarina Mar 8, 2019

Collaborator

The reason this wasn't included for these test data before was that the Charts themselves don't define an AppVersion so this is going to set it to an empty string. It's ok not to include something if there will be no value. This thought applies to all the label changes on this particular testdata chart.

This comment has been minimized.

Copy link
@luisdavim

luisdavim Mar 9, 2019

Author Contributor

I've added appVersion to the Chart.yaml

@@ -114,6 +114,7 @@ metadata:
# I cannot reference .Chart.Name, but I can do $.Chart.Name
helm.sh/chart: "{{ $.Chart.Name }}-{{ $.Chart.Version }}"
app.kubernetes.io/instance: "{{ $.Release.Name }}"
app.kubernetes.io/version: {{ .Chart.AppVersion }}

This comment has been minimized.

Copy link
@mattfarina

mattfarina Mar 8, 2019

Collaborator

Can you please add a comment above this to advice people to use the app.kubernetes.io/version if there is an AppVersion in their Chart.yaml file.

@@ -6,6 +6,7 @@ metadata:
labels:
app.kubernetes.io/managed-by: {{ .Release.Service }}
app.kubernetes.io/instance: {{ .Release.Name }}
app.kubernetes.io/version: {{ .Chart.AppVersion }}

This comment has been minimized.

Copy link
@mattfarina

mattfarina Mar 8, 2019

Collaborator

Before using AppVersion here can you add it to the Chart.yaml so there will be a value.

{{- define "<CHARTNAME>.labels" -}}
app.kubernetes.io/name: {{ include "<CHARTNAME>.name" . }}
helm.sh/chart: {{ include "<CHARTNAME>.chart" . }}
app.kubernetes.io/instance: {<CHARTNAME>se.Name }}

This comment has been minimized.

Copy link
@mattfarina

mattfarina Mar 8, 2019

Collaborator

I think this is supposed to be .Release.Name

@@ -7,6 +7,7 @@ metadata:
labels:
app.kubernetes.io/managed-by: {{ .Release.Service | quote }}
app.kubernetes.io/instance: {{ .Release.Name | quote }}
app.kubernetes.io/version: {{ .Chart.AppVersion }}

This comment has been minimized.

Copy link
@mattfarina

mattfarina Mar 8, 2019

Collaborator

The Chart.yaml should include a version if it's going to be called here. We should either omit the AppVersion or add it to the Chart.yaml

Add app.kubernetes.io/version label
Signed-off-by: Luis Davim <luis.davim@jet.com>

@luisdavim luisdavim force-pushed the luisdavim:version_label branch from ce8f1f6 to a274e96 Mar 9, 2019

@helm-bot helm-bot added size/M and removed size/M labels Mar 9, 2019

@luisdavim luisdavim force-pushed the luisdavim:version_label branch from a274e96 to 048d06a Mar 9, 2019

@helm-bot helm-bot added size/M and removed size/M labels Mar 9, 2019

@luisdavim luisdavim force-pushed the luisdavim:version_label branch from 048d06a to f5f250e Mar 9, 2019

@helm-bot helm-bot added size/M and removed size/M labels Mar 9, 2019

@luisdavim

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2019

Thanks for the review @mattfarina I think I've addressed all your comments.

@mattfarina

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

@luisdavim Can you please fix the tests?

@luisdavim

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

sure, I'll try to get that done by the end of the week.

@luisdavim luisdavim force-pushed the luisdavim:version_label branch 4 times, most recently from ab5e064 to 3b6cb0c Apr 18, 2019

@luisdavim

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

it was easier than I thought, they're fixed now.

@mattfarina
Copy link
Collaborator

left a comment

Thanks for making the changes. I have just one more nit for doc consistency...

@@ -114,6 +114,8 @@ metadata:
# I cannot reference .Chart.Name, but I can do $.Chart.Name
helm.sh/chart: "{{ $.Chart.Name }}-{{ $.Chart.Version }}"
app.kubernetes.io/instance: "{{ $.Release.Name }}"
# Value from appVersion in Chart.yaml
app.kubernetes.io/version: "{{ .Chart.AppVersion }}"

This comment has been minimized.

Copy link
@mattfarina

mattfarina Apr 23, 2019

Collaborator

This is a nit but the other docs in this section start with $.Chart.... This instance should hold the pattern to be consistent. Can you please update it.

This comment has been minimized.

Copy link
@luisdavim

luisdavim Apr 24, 2019

Author Contributor

updated.

Reduce template code duplication. Fixes #5372
Signed-off-by: Luis Davim <luis.davim@jet.com>

@luisdavim luisdavim force-pushed the luisdavim:version_label branch from 3b6cb0c to 0270f2e Apr 24, 2019

@mattfarina mattfarina merged commit b06b5ef into helm:master Apr 24, 2019

2 checks passed

DCO DCO
Details
ci/circleci: build Your tests passed on CircleCI!
Details

@mattfarina mattfarina referenced this pull request May 7, 2019

Merged

Add app version #5684

0 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.