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

Splitting jsonpatches on a conditional antrea installation for windows #3867

Merged
merged 1 commit into from
Nov 10, 2022

Conversation

knabben
Copy link
Member

@knabben knabben commented Nov 8, 2022

What this PR does / why we need it

Splitting conditional JSON patches for Antrea and Windows installations, this will allow operator to use CNI:none and do not have antrea by default in the windows node.

Which issue(s) this PR fixes

Fixes #3866

Describe testing done for PR

Manual tests with the CC were made

Release note

CNI:none enabled on ClusterClass Windows nodes.

Additional information

Special notes for your reviewer

@knabben knabben requested a review from a team as a code owner November 8, 2022 14:34
@knabben knabben force-pushed the topic/knabben/windows-antrea-patch branch 2 times, most recently from 5dcd360 to 7826048 Compare November 8, 2022 15:26
@knabben knabben requested review from a team as code owners November 8, 2022 15:26
@github-actions
Copy link

github-actions bot commented Nov 8, 2022

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3867/20221108153619/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@knabben knabben force-pushed the topic/knabben/windows-antrea-patch branch from 7826048 to f480546 Compare November 8, 2022 17:07
@github-actions
Copy link

github-actions bot commented Nov 8, 2022

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3867/20221108171650/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@codecov
Copy link

codecov bot commented Nov 8, 2022

Codecov Report

Merging #3867 (702bc11) into main (2f9044a) will decrease coverage by 0.90%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3867      +/-   ##
==========================================
- Coverage   46.93%   46.03%   -0.91%     
==========================================
  Files         402      427      +25     
  Lines       40203    41754    +1551     
==========================================
+ Hits        18869    19221     +352     
- Misses      19582    20765    +1183     
- Partials     1752     1768      +16     
Impacted Files Coverage Δ
cmd/cli/plugin/cluster/scale.go 17.85% <0.00%> (ø)
cmd/cli/plugin/cluster/set_machinehealthcheck.go 23.33% <0.00%> (ø)
...in/cluster/get_machinehealthcheck_control_plane.go 11.11% <0.00%> (ø)
...cluster/delete_machinehealthcheck_control_plane.go 16.66% <0.00%> (ø)
.../cli/plugin/cluster/get_machinehealthcheck_node.go 9.30% <0.00%> (ø)
cmd/cli/plugin/cluster/osimage.go 100.00% <0.00%> (ø)
cmd/cli/plugin/cluster/kubeconfig_get.go 8.82% <0.00%> (ø)
cmd/cli/plugin/cluster/list.go 11.36% <0.00%> (ø)
...in/cluster/set_machinehealthcheck_control_plane.go 21.21% <0.00%> (ø)
cmd/cli/plugin/cluster/delete.go 12.50% <0.00%> (ø)
... and 16 more

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

@knabben
Copy link
Member Author

knabben commented Nov 8, 2022

/test install-vc7

@alfredthenarwhal
Copy link
Collaborator

Tests failed! Build no: 3158

@vuil
Copy link
Contributor

vuil commented Nov 8, 2022

cc @tenczar to take a look.
@knabben can you add what testing was done with this change to the PR description. It seems at minimum we need to test the effect of CNI:none on windows and non-windows nodes.

Could you also have demonstrated the effect of these changes using clustergen?

Copy link
Contributor

@tenczar tenczar left a comment

Choose a reason for hiding this comment

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

LGTM, One change, please bump the annotation at line 174 of base.yml identifier.cc.vmware.com: tanzu-vsphere-default-0.0.2 this version is used by the UI team to track if changes have been made to the variable schema.

@wjun wjun added the ok-to-merge PRs should be labelled with this before merging label Nov 9, 2022
@knabben knabben force-pushed the topic/knabben/windows-antrea-patch branch from f480546 to 649523a Compare November 9, 2022 13:08
@wjun wjun removed the ok-to-merge PRs should be labelled with this before merging label Nov 9, 2022
@github-actions
Copy link

github-actions bot commented Nov 9, 2022

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3867/20221109131757/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@knabben knabben force-pushed the topic/knabben/windows-antrea-patch branch from 649523a to 702bc11 Compare November 9, 2022 14:29
@github-actions
Copy link

github-actions bot commented Nov 9, 2022

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3867/20221109143942/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@wjun wjun added the ok-to-merge PRs should be labelled with this before merging label Nov 10, 2022
@knabben knabben merged commit 1ab6021 into main Nov 10, 2022
@knabben knabben deleted the topic/knabben/windows-antrea-patch branch November 10, 2022 09:29
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.

Enable the CNI:none option on Windows nodes
6 participants