Create separate go modules for `apis/cli` and `apis/config` #3141

anujc25 · 2022-08-16T05:23:01Z

What this PR does / why we need it

Create separate go modules for apis/cli
Create separate go modules for apis/config

Which issue(s) this PR fixes

Describe testing done for PR

Release note

Create separate go modules for `apis/cli` and `apis/config`

Additional information

Special notes for your reviewer

github-actions · 2022-08-16T05:30:41Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3141/20220816053039/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.

marckhouzam · 2022-08-16T14:00:50Z

This looks great @anujc25.

Comparing the apis/cli and apis/config with the existing apis/cni and apis/cpi, I notice that the latter both have a Makefile and are part of the main Makefile manifests target. These Makefiles were added to fix #2398. I don't understand the controller-gen concept at the moment, but I thought I'd mention it, in case we need the same thing for the new go modules.

anujc25 · 2022-08-16T17:30:46Z

Comparing the apis/cli and apis/config with the existing apis/cni and apis/cpi, I notice that the latter both have a Makefile and are part of the main Makefile manifests target. These Makefiles were added to fix #2398. I don't understand the controller-gen concept at the moment, but I thought I'd mention it, in case we need the same thing for the new go modules.

Thanks. Great catch. Updated the PR to update Makefiles.

github-actions · 2022-08-16T17:41:58Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3141/20220816174156/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 · 2022-08-16T18:18:33Z

Codecov Report

Merging #3141 (ba90d37) into main (0086f33) will increase coverage by 0.46%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main    #3141      +/-   ##
==========================================
+ Coverage   44.46%   44.93%   +0.46%     
==========================================
  Files         417      412       -5     
  Lines       42399    41722     -677     
==========================================
- Hits        18851    18746     -105     
+ Misses      21818    21252     -566     
+ Partials     1730     1724       -6

Impacted Files	Coverage Δ
cmd/cli/plugin/cluster/create.go	`41.30% <0.00%> (-0.92%)`	⬇️
cmd/cli/plugin/cluster/upgrade.go	`57.55% <0.00%> (-0.96%)`	⬇️
pkg/v1/cli/command/core/config.go	`36.15% <ø> (-0.32%)`	⬇️
pkg/v1/tkg/tkgctl/compatibility.go	`70.00% <70.00%> (ø)`
addons/controllers/clusterbootstrap_controller.go	`63.28% <0.00%> (-2.30%)`	⬇️
apis/config/v1alpha1/clientconfig.go
apis/config/v1alpha1/featuregate_webhook.go
apis/config/v1alpha1/clientconfig_types.go
apis/config/v1alpha1/zz_generated.deepcopy.go
apis/config/v1alpha1/feature_types.go
... and 4 more

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

github-actions · 2022-08-16T19:42:16Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3141/20220816194214/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.

github-actions · 2022-08-16T19:47:07Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3141/20220816194705/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.

github-actions · 2022-08-16T20:17:15Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3141/20220816201713/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.

apis/config/v1alpha1/clientconfig.go

anujc25 · 2022-08-16T20:23:54Z

pkg/v1/cli/command/core/config.go

@@ -190,12 +190,6 @@ func setEdition(cfg *configv1alpha1.ClientConfig, edition string) error {
 	switch editionOption {
 	case configv1alpha1.EditionCommunity, configv1alpha1.EditionStandard:
 		cfg.SetEditionSelector(editionOption)
-		// when community edition is set, configure the compatibility file to use
-		// community edition's.
-		err := cfg.SetCompatibilityFile(editionOption)


This was removed from core-cli code and moved to individual plugin command invocations.

Ok, now I'm completely out of my element, but please be patient with me.

I was curious about the change in logic about SetCompatibilityFile. Looking at the change, I'm guessing that this compatibility file is only used when creating a cluster? That is why you only need to trigger it for the managed-cluster and cluster plugins?

But somehow (probably me doing something wrong), the main configuration file does not get updated properly anymore. This is a simplification of what I did:

# Check that the main config file is set to tkg (BOM and edition) $ egrep 'bom|edition' ~/.config/tanzu/config.yaml bomRepo: projects-stg.registry.vmware.com/tkg edition: tkg # Set the edition to TCE $ tanzu config set cli.edition tce # =====> Notice the BOM is no longer set properly $ egrep 'bom|edition' ~/.config/tanzu/config.yaml bomRepo: projects-stg.registry.vmware.com/tkg edition: tce # Remove the BOM line completely $ grep -v bom ~/.config/tanzu/config.yaml > tt && \mv tt ~/.config/tanzu/config.yaml && grep bom ~/.config/tanzu/config.yaml # Regenerate the config file by running any command $ tanzu Tanzu CLI Usage: tanzu [command] [...] # =====> Notice the BOM is added but does not match the edition $ egrep 'bom|edition' ~/.config/tanzu/config.yaml bomRepo: projects-stg.registry.vmware.com/tkg edition: tce

I don't know if this is actually a problem but it does not happen on the main branch.

Thanks for catching that. I have fixed the problem and added unit test around SetCompatibilityFileBasedOnEdition function as well.

Please note that tanzu config set cli.edition tce command will now just update the edition field in config and BomRepo and Compatibility information is updated when user runs cluster or management-cluster plugin command. Ideally, BomRepo and Compatibility information should not be part of ClientConfig at the first place and I have filled #3149 for that.

github-actions · 2022-08-16T20:42:21Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3141/20220816204219/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.

marckhouzam

Sorry if I'm all over the place, this is still pretty new to me...

apis/config/v1alpha1/featuregate_webhook_test.go

apis/config/v1alpha1/featuregate_webhook.go

apis/config/v1alpha1/featuregate_webhook_test.go

pkg/v1/sdk/capabilities/Dockerfile

marckhouzam · 2022-08-16T20:39:36Z

pkg/v1/cli/command/core/config.go

@@ -190,12 +190,6 @@ func setEdition(cfg *configv1alpha1.ClientConfig, edition string) error {
 	switch editionOption {
 	case configv1alpha1.EditionCommunity, configv1alpha1.EditionStandard:
 		cfg.SetEditionSelector(editionOption)
-		// when community edition is set, configure the compatibility file to use
-		// community edition's.
-		err := cfg.SetCompatibilityFile(editionOption)


Ok, now I'm completely out of my element, but please be patient with me.

I was curious about the change in logic about SetCompatibilityFile. Looking at the change, I'm guessing that this compatibility file is only used when creating a cluster? That is why you only need to trigger it for the managed-cluster and cluster plugins?

But somehow (probably me doing something wrong), the main configuration file does not get updated properly anymore. This is a simplification of what I did:

# Check that the main config file is set to tkg (BOM and edition) $ egrep 'bom|edition' ~/.config/tanzu/config.yaml bomRepo: projects-stg.registry.vmware.com/tkg edition: tkg # Set the edition to TCE $ tanzu config set cli.edition tce # =====> Notice the BOM is no longer set properly $ egrep 'bom|edition' ~/.config/tanzu/config.yaml bomRepo: projects-stg.registry.vmware.com/tkg edition: tce # Remove the BOM line completely $ grep -v bom ~/.config/tanzu/config.yaml > tt && \mv tt ~/.config/tanzu/config.yaml && grep bom ~/.config/tanzu/config.yaml # Regenerate the config file by running any command $ tanzu Tanzu CLI Usage: tanzu [command] [...] # =====> Notice the BOM is added but does not match the edition $ egrep 'bom|edition' ~/.config/tanzu/config.yaml bomRepo: projects-stg.registry.vmware.com/tkg edition: tce

I don't know if this is actually a problem but it does not happen on the main branch.

prkalle

The changes looks good

github-actions · 2022-08-17T01:55:01Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3141/20220817015458/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.

github-actions · 2022-08-17T03:50:05Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3141/20220817035002/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.

github-actions · 2022-08-17T04:37:33Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3141/20220817043732/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.

marckhouzam

Thanks for your patience and efforts @anujc25. I think this will pave the way to easier future refactorings for everyone else.

The new tests have to be fixed (they don't compile), but the rest looks great 👍

pkg/v1/sdk/capabilities/Dockerfile

cmd/cli/plugin/managementcluster/delete.go

config/crd/bases/cni.tanzu.vmware.com_antreaconfigs.yaml

github-actions · 2022-08-17T15:43:46Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3141/20220817154345/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.

…ncies

Signed-off-by: Anuj Chaudhari <anujc@vmware.com>

github-actions · 2022-08-17T16:31:22Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3141/20220817163120/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.

vuil

lgtm, thanks!

anujc25 requested review from a team as code owners August 16, 2022 05:23

vmwclabot added the cla-not-required label Aug 16, 2022

anujc25 force-pushed the separate-go-mod-apis-cli-config branch from be97f28 to dd97d76 Compare August 16, 2022 17:28

anujc25 force-pushed the separate-go-mod-apis-cli-config branch from dd97d76 to 08738f8 Compare August 16, 2022 19:30

anujc25 requested a review from prkalle as a code owner August 16, 2022 19:30

anujc25 force-pushed the separate-go-mod-apis-cli-config branch from 08738f8 to 851c4a1 Compare August 16, 2022 19:35

anujc25 force-pushed the separate-go-mod-apis-cli-config branch from 851c4a1 to 5dcec40 Compare August 16, 2022 20:06

anujc25 commented Aug 16, 2022

View reviewed changes

apis/config/v1alpha1/clientconfig.go Show resolved Hide resolved

anujc25 commented Aug 16, 2022

View reviewed changes

anujc25 force-pushed the separate-go-mod-apis-cli-config branch from 5dcec40 to 224687b Compare August 16, 2022 20:32

marckhouzam reviewed Aug 16, 2022

View reviewed changes

prkalle reviewed Aug 16, 2022

View reviewed changes

anujc25 force-pushed the separate-go-mod-apis-cli-config branch from 224687b to bcead3e Compare August 17, 2022 01:44

anujc25 force-pushed the separate-go-mod-apis-cli-config branch from bcead3e to a1ac38c Compare August 17, 2022 03:37

anujc25 force-pushed the separate-go-mod-apis-cli-config branch from a1ac38c to 9299bd2 Compare August 17, 2022 04:27

anujc25 assigned rajathagasthya Aug 17, 2022

anujc25 assigned vuil and anujc25 and unassigned anujc25 Aug 17, 2022

marckhouzam approved these changes Aug 17, 2022

View reviewed changes

pkg/v1/sdk/capabilities/Dockerfile Show resolved Hide resolved

cmd/cli/plugin/managementcluster/delete.go Show resolved Hide resolved

config/crd/bases/cni.tanzu.vmware.com_antreaconfigs.yaml Show resolved Hide resolved

anujc25 force-pushed the separate-go-mod-apis-cli-config branch from 9299bd2 to 3767818 Compare August 17, 2022 15:33

anujc25 added 4 commits August 17, 2022 12:20

Remove dependency of pkg/v1 from apis/config/v1alpha1

646c14d

Make apis/cli and apis/config a separate go module and update depende…

8ade76f

…ncies

Update go.mod and build dependency in other modules

86512bd

Signed-off-by: Anuj Chaudhari <anujc@vmware.com>

Sync controller-gen manifests

ba90d37

Signed-off-by: Anuj Chaudhari <anujc@vmware.com>

anujc25 force-pushed the separate-go-mod-apis-cli-config branch from 3767818 to ba90d37 Compare August 17, 2022 16:21

vuil approved these changes Aug 17, 2022

View reviewed changes

anujc25 added ok-to-merge PRs should be labelled with this before merging kind/refactor area/core-cli labels Aug 18, 2022

anujc25 merged commit 482ee91 into vmware-tanzu:main Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create separate go modules for `apis/cli` and `apis/config` #3141

Create separate go modules for `apis/cli` and `apis/config` #3141

anujc25 commented Aug 16, 2022

github-actions bot commented Aug 16, 2022

marckhouzam commented Aug 16, 2022

anujc25 commented Aug 16, 2022

github-actions bot commented Aug 16, 2022

codecov bot commented Aug 16, 2022 •

edited

Loading

github-actions bot commented Aug 16, 2022

github-actions bot commented Aug 16, 2022

github-actions bot commented Aug 16, 2022

anujc25 Aug 16, 2022

marckhouzam Aug 16, 2022

anujc25 Aug 17, 2022

github-actions bot commented Aug 16, 2022

marckhouzam left a comment

marckhouzam Aug 16, 2022

prkalle left a comment

github-actions bot commented Aug 17, 2022

github-actions bot commented Aug 17, 2022

github-actions bot commented Aug 17, 2022

marckhouzam left a comment

github-actions bot commented Aug 17, 2022

github-actions bot commented Aug 17, 2022

vuil left a comment

Create separate go modules for apis/cli and apis/config #3141

Create separate go modules for apis/cli and apis/config #3141

Conversation

anujc25 commented Aug 16, 2022

What this PR does / why we need it

Which issue(s) this PR fixes

Describe testing done for PR

Release note

Additional information

Special notes for your reviewer

github-actions bot commented Aug 16, 2022

marckhouzam commented Aug 16, 2022

anujc25 commented Aug 16, 2022

github-actions bot commented Aug 16, 2022

codecov bot commented Aug 16, 2022 • edited Loading

Codecov Report

github-actions bot commented Aug 16, 2022

github-actions bot commented Aug 16, 2022

github-actions bot commented Aug 16, 2022

anujc25 Aug 16, 2022

Choose a reason for hiding this comment

marckhouzam Aug 16, 2022

Choose a reason for hiding this comment

anujc25 Aug 17, 2022

Choose a reason for hiding this comment

github-actions bot commented Aug 16, 2022

marckhouzam left a comment

Choose a reason for hiding this comment

marckhouzam Aug 16, 2022

Choose a reason for hiding this comment

prkalle left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 17, 2022

github-actions bot commented Aug 17, 2022

github-actions bot commented Aug 17, 2022

marckhouzam left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 17, 2022

github-actions bot commented Aug 17, 2022

vuil left a comment

Choose a reason for hiding this comment

Create separate go modules for `apis/cli` and `apis/config` #3141

Create separate go modules for `apis/cli` and `apis/config` #3141

codecov bot commented Aug 16, 2022 •

edited

Loading