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

Fix canary to primary label promotion #1405

Merged
merged 1 commit into from
Apr 10, 2023
Merged

Fix canary to primary label promotion #1405

merged 1 commit into from
Apr 10, 2023

Conversation

ta924
Copy link
Contributor

@ta924 ta924 commented Apr 5, 2023 •
edited by stefanprodan
Loading

This PR is to address issue #1403 where all canary labels are copied to the primary on promote. This resulting in the following issues

  • primary label was lost on promotion
  • if no value was provided for include-label-prefix all labels would be copied over

Fix: #1403

@ta924 ta924 requested a review from stefanprodan as a code owner April 5, 2023 18:40
@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.05 🎉

Comparison is base (f3f6266) 54.42% compared to head (8b83de5) 54.47%.

❗ Current head 8b83de5 differs from pull request most recent head 44363d5. Consider uploading reports for the commit 44363d5 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1405      +/-   ##
==========================================
+ Coverage   54.42%   54.47%   +0.05%     
==========================================
  Files          84       84              
  Lines       10049    10047       -2     
==========================================
+ Hits         5469     5473       +4     
+ Misses       3925     3921       -4     
+ Partials      655      653       -2
Impacted Files Coverage Δ
pkg/canary/daemonset_controller.go 57.14% <100.00%> (+1.10%) ⬆️
pkg/canary/deployment_controller.go 44.33% <100.00%> (+0.60%) ⬆️
pkg/canary/util.go 55.00% <100.00%> (ø)
pkg/router/util.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ta924
Copy link
Contributor Author

ta924 commented Apr 6, 2023

@stefanprodan and feedback on resolving the failed e2e test? Seems to be a versioning issue with k8s

aryan9600
aryan9600 requested changes Apr 7, 2023
View reviewed changes
Copy link
Member

@aryan9600 aryan9600 left a comment

Choose a reason for hiding this comment

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

@ta924 thanks for opening this PR! just have a couple of nits.

pkg/canary/util_test.go Outdated Show resolved Hide resolved
pkg/canary/deployment_controller.go Show resolved Hide resolved
@ta924
address issue with all canary labels copied to primary on promote
44363d5 
address issue with all canary labels copied to primary on promote

Signed-off-by: ta924@yahoo.com <ta924@yahoo.com>

address review comments
aryan9600
aryan9600 approved these changes Apr 10, 2023
View reviewed changes
Copy link
Member

@aryan9600 aryan9600 left a comment

Choose a reason for hiding this comment

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

lgtm! thanks @ta924 🙇

stefanprodan
stefanprodan approved these changes Apr 10, 2023
View reviewed changes
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

@aryan9600 aryan9600 changed the title address issue with all canary labels copied to primary on promote avoid copying canary labels to primary on promotion Apr 10, 2023
@stefanprodan stefanprodan changed the title avoid copying canary labels to primary on promotion Fix canary to primary label promotion Apr 10, 2023
@aryan9600 aryan9600 merged commit f1def19 into fluxcd:main Apr 10, 2023
@ta924
Copy link
Contributor Author

ta924 commented Apr 10, 2023

thank you @aryan9600 and @stefanprodan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefanprodan stefanprodan stefanprodan approved these changes

@aryan9600 aryan9600 aryan9600 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Canary labels copied to primary on promotion when include-label-prefix not defined
4 participants
@ta924 @codecov-commenter @stefanprodan @aryan9600