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 two new labels #3184

Merged
merged 3 commits into from
Aug 15, 2023
Merged

Add two new labels #3184

merged 3 commits into from
Aug 15, 2023

Conversation

matthchr
Copy link
Member

  • Add the app.kubernetes.io/name and app.kubernetes.io/version labels.
  • Remove serviceoperator.azure.com/version in favor of the standard app.kubernetes.io/version label.
  • Update taskfile to use new labels in various places.

This fixes #3165.

If applicable:

  • this PR contains documentation
  • this PR contains tests

@codecov-commenter
Copy link

codecov-commenter commented Aug 10, 2023

Codecov Report

Merging #3184 (629c452) into main (abc6f01) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3184      +/-   ##
==========================================
+ Coverage   54.39%   54.40%   +0.01%     
==========================================
  Files        1424     1424              
  Lines      601364   601364              
==========================================
+ Hits       327122   327189      +67     
+ Misses     220776   220690      -86     
- Partials    53466    53485      +19     
Files Changed Coverage Δ
v2/internal/crdmanagement/manager.go 61.53% <100.00%> (ø)

... and 34 files with indirect coverage changes

 - Add the app.kubernetes.io/name and app.kubernetes.io/version labels.
 - Remove serviceoperator.azure.com/version in favor of the standard
   app.kubernetes.io/version label.
 - Update taskfile to use new labels in various places.

This fixes Azure#3165.
@super-harsh
Copy link
Collaborator

Would be good to open an issue to remove the old label at some point?

@matthchr
Copy link
Member Author

We can't easily remove the old label because it's being used as a labelSelector for the deployment. Technically speaking, we could use these new labels as selectors and remove the old one, but:

  1. controller-runtime projects all have this label, so I think it's OK for us to keep it.
  2. Changing matchLabels for a deployment cannot be done without deleting and recreating the deployment. In Helm, this requires use of the --force flag which the users must specify.

Given that there aren't any other pods in our namespace which use this label, I think it's OK to have this be our label selector and avoid making users do a --force. We can always decide later to do this but I don't see all that much of a win now.

@matthchr
Copy link
Member Author

/ok-to-test sha=fe16343

@matthchr
Copy link
Member Author

/ok-to-test sha=629c452

@matthchr matthchr merged commit 366a050 into Azure:main Aug 15, 2023
8 checks passed
@matthchr matthchr deleted the feature/new-label branch August 15, 2023 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Deployment should have an additional label
3 participants