Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

Add missing labels for antreaconfig from clusterbootstrap #4555

Merged
merged 5 commits into from
Apr 7, 2023

Conversation

hangyan
Copy link
Contributor

@hangyan hangyan commented Apr 7, 2023

What this PR does / why we need it

Which issue(s) this PR fixes

In the last version bump, antrea controller relies on a package label to working normally. But in cert cases(cb and antreaconfig created from kubectl, the label is missing). This patch added the missing label from clusterboostrap clone

Fixes # TKG-18782

This is also an alternative solution compared to #4544

Describe testing done for PR

Test done in a Live tanzu env

Release note


Additional information

Special notes for your reviewer

Signed-off-by: Hang Yan <[email protected]>
@codecov
Copy link

codecov bot commented Apr 7, 2023

Codecov Report

Merging #4555 (82e8782) into main (7cf3455) will decrease coverage by 0.88%.
The diff coverage is 42.22%.

@@            Coverage Diff             @@
##             main    #4555      +/-   ##
==========================================
- Coverage   49.78%   48.91%   -0.88%     
==========================================
  Files         453      483      +30     
  Lines       45379    47544    +2165     
==========================================
+ Hits        22594    23255     +661     
- Misses      20632    22082    +1450     
- Partials     2153     2207      +54     
Impacted Files Coverage Δ
addons/controllers/clusterbootstrap_controller.go 65.95% <0.00%> (+1.30%) ⬆️
...til/clusterbootstrapclone/clusterbootstrapclone.go 66.13% <46.34%> (-0.98%) ⬇️

... and 35 files with indirect coverage changes

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

Signed-off-by: Hang Yan <[email protected]>
Signed-off-by: Hang Yan <[email protected]>
@hangyan
Copy link
Contributor Author

hangyan commented Apr 7, 2023

@wjun Another issue is that this is an alternative solution as #4544. 4544 is a more general solution, it will update all the providers.

Copy link
Contributor

@shyaamsn shyaamsn left a comment

Choose a reason for hiding this comment

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

LGTM. Please ensure all the tests in CAPA pipeline passes. Then this can be merged.

Copy link
Contributor

@12345lcr 12345lcr left a comment

Choose a reason for hiding this comment

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

LGTM

@shyaamsn
Copy link
Contributor

shyaamsn commented Apr 7, 2023

We also need to update the label in the upgrade case. Need to call this same method to update label during upgrade here https://github.com/vmware-tanzu/tanzu-framework/blob/main/addons/controllers/clusterbootstrap_controller.go#L431

@shyaamsn shyaamsn self-requested a review April 7, 2023 11:14
@hangyan
Copy link
Contributor Author

hangyan commented Apr 7, 2023

We also need to update the label in the upgrade case. Need to call this same method to update label during upgrade here https://github.com/vmware-tanzu/tanzu-framework/blob/main/addons/controllers/clusterbootstrap_controller.go#L431

updated.

Copy link
Contributor

@shyaamsn shyaamsn left a comment

Choose a reason for hiding this comment

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

LGTM

@shyaamsn shyaamsn added the ok-to-merge PRs should be labelled with this before merging label Apr 7, 2023
@hangyan hangyan merged commit bb07e03 into main Apr 7, 2023
@hangyan hangyan deleted the topic/yhang/fix-antreaconfig-labels branch April 7, 2023 14:13
@shyaamsn
Copy link
Contributor

shyaamsn commented Apr 7, 2023

/test install-vc7,upgrade-vc7

@alfredthenarwhal
Copy link
Collaborator

@shyaamsn: /test install-vc7,upgrade-vc7
Commit: 82e8782

Tests failed! Build no: 4728

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-not-required ok-to-merge PRs should be labelled with this before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants