10BC0 Fix Helm Chart labels and templates. Move version update to labels by lucacome · Pull Request #3606 · nginx/kubernetes-ingress · GitHub
[go: up one dir, main page]

Skip to content

Conversation

lucacome
Copy link
@lucacome lucacome commented Mar 1, 2023

app.kubernetes.io/version and app.nginx.org/version should be labels and not annotations.

This also fixes name generation for the Helm Chart and adds selector labels.

Removed unnecessary names.

@lucacome lucacome requested a review from a team as a code owner March 1, 2023 04:15
@lucacome lucacome self-assigned this Mar 1, 2023
@github-actions github-actions bot added chore Pull requests for routine tasks helm_chart Pull requests that update the Helm Chart labels Mar 1, 2023
Copy link
Contributor
@jasonwilliams14 jasonwilliams14 left a comment

Choose a reason for hiding this comment

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

Tested settings successfully.

@codecov-commenter
Copy link
codecov-commenter commented Mar 1, 2023

Codecov Report

Merging #3606 (ab9d924) into main (3ff67a4) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #3606      +/-   ##
==========================================
- Coverage   52.26%   52.24%   -0.02%     
==========================================
  Files          59       59              
  Lines       16873    16873              
==========================================
- Hits         8818     8816       -2     
- Misses       7760     7762       +2     
  Partials      295      295              
Impacted Files Coverage Δ
cmd/nginx-ingress/main.go 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@lucacome lucacome force-pushed the chore/fix-app-labels branch from 41ab37d to 3535829 Compare March 2, 2023 02:57
Copy link
Contributor
@jasonwilliams14 jasonwilliams14 left a comment

Choose a reason for hiding this comment

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

Tested and looks good.

@lucacome lucacome changed the title Fix Helm Chart labels and version update, add selector labels @lucacome Fix Helm Chart labels and templates. Move version update to labels Mar 2, 2023
@lucacome lucacome changed the title @lucacome Fix Helm Chart labels and templates. Move version update to labels Fix Helm Chart labels and templates. Move version update to labels Mar 2, 2023
@lucacome lucacome force-pushed the chore/fix-app-labels branch from 38ea760 to 9041aa1 Compare March 2, 2023 17:55
@lucacome lucacome requested a review from ciarams87 March 3, 2023 03:26
Copy link
Contributor
@ciarams87 ciarams87 left a comment

Choose a reason for hiding this comment

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

When I try this locally, the version labels don't update, and I get the following in the logs:

Error updating pod with labels: Pod "my-chart-nginx-ingress-controller-75c4fc6d5d-fxl7l" is invalid: [metadata.labels: Invalid value: ""3.0.0-SNAPSHOT-dda91a4"": a valid label must be an empty string or consist of alphanumeric characters, '-', '' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9.])?[A-Za-z0-9])?'), metadata.labels: Invalid value: ""1.23.2 (nginx-plus-r28)"": a valid label must be an empty string or consist of alphanumeric characters, '-', '' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9.])?[A-Za-z0-9])?')]

@lucacome lucacome force-pushed the chore/fix-app-labels branch from dda91a4 to 7665779 Compare March 9, 2023 01:13
@lucacome
Copy link
Author
lucacome commented Mar 9, 2023

Thanks for catching this @ciarams87 ! I was doing some tests with quoting the versions and apparently, it was double quoting the string now. I've fixed it.

@lucacome lucacome requested a review from ciarams87 March 9, 2023 01:16
Copy link
Contributor
@ciarams87 ciarams87 left a comment

Choose a reason for hiding this comment

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

LGTM now, thanks!

"app.kubernetes.io/version" and "app.nginx.org/version" should be labels
and not annotations.

This also fixes name generation for the Helm Chart and adds selector
labels.
@lucacome lucacome force-pushed the chore/fix-app-labels branch from ab9d924 to 088c150 Compare March 14, 2023 23:28
@lucacome lucacome merged commit 96077f9 into main Mar 15, 2023
@lucacome lucacome deleted the chore/fix-app-labels branch March 15, 2023 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Pull requests for routine tasks helm_chart Pull requests that update the Helm Chart
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0