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

Refactor cmd/cli/plugin/package: create its own go.mod and move in openapischema #3316

Merged
merged 2 commits into from
Sep 20, 2022

Conversation

codegold79
Copy link
Contributor

What this PR does / why we need it

  • Since no other package outside of package plugin uses openapischema, bring it it into the cmd/cli/plugin/package directory
  • Create go.mod and go.sum files for cmd/cli/plugin/package
  • Update Tanzu Framework project root go.mod and go.sum files

Which issue(s) this PR fixes

Fixes #

Describe testing done for PR

I was able to run make build-cli-local successfully. I put the output here: make build-cli-local.txt

Release note

Refactor cmd/cli/plugin/package to have its own go.{mod,sum} and contain openapischema package

Additional information

Special notes for your reviewer

@codegold79 codegold79 requested a review from maralavi September 12, 2022 22:39
@codegold79 codegold79 self-assigned this Sep 12, 2022
@codegold79 codegold79 requested review from a team as code owners September 12, 2022 22:39
@codegold79 codegold79 marked this pull request as draft September 12, 2022 22:44
@codegold79 codegold79 force-pushed the package-plugin-refactoring-2nd-try branch from 8a37bfb to 45244f1 Compare September 13, 2022 00:35
@codegold79 codegold79 changed the title Refactor cmd/cli/plugin/package: move in openapischema and create its own go mod Refactor cmd/cli/plugin/package: create its own go.mod and move in openapischema Sep 13, 2022
@codegold79 codegold79 force-pushed the package-plugin-refactoring-2nd-try branch 4 times, most recently from aad3484 to bf74a73 Compare September 13, 2022 01:37
@codecov
Copy link

codecov bot commented Sep 13, 2022

Codecov Report

Merging #3316 (52cc6d3) into main (8a526ce) will increase coverage by 5.98%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3316      +/-   ##
==========================================
+ Coverage   46.91%   52.90%   +5.98%     
==========================================
  Files         265      103     -162     
  Lines       28879    10419   -18460     
==========================================
- Hits        13550     5512    -8038     
+ Misses      14082     4446    -9636     
+ Partials     1247      461     -786     
Impacted Files Coverage Δ
cmd/cli/plugin/cluster/available_upgrade.go 16.32% <ø> (ø)
cmd/cli/plugin/cluster/create.go 41.30% <ø> (ø)
cmd/cli/plugin/cluster/credentials_update.go 9.23% <ø> (ø)
cmd/cli/plugin/cluster/delete.go 12.50% <ø> (ø)
...md/cli/plugin/cluster/delete_machinehealthcheck.go 19.23% <ø> (ø)
...cluster/delete_machinehealthcheck_control_plane.go 16.66% <ø> (ø)
...i/plugin/cluster/delete_machinehealthcheck_node.go 16.66% <ø> (ø)
cmd/cli/plugin/cluster/delete_node_pool.go 16.66% <ø> (ø)
cmd/cli/plugin/cluster/get.go 6.27% <ø> (ø)
cmd/cli/plugin/cluster/get_machinehealthcheck.go 11.42% <ø> (ø)
... and 184 more

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

@codegold79 codegold79 force-pushed the package-plugin-refactoring-2nd-try branch from bf74a73 to 7e0e6df Compare September 13, 2022 17:22
@github-actions
Copy link

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

@maralavi
Copy link
Contributor

maralavi commented Sep 13, 2022

I have a question for @vijaykatam @vuil @anujc25 @shyaamsn here:

The cmd/cli/plugin/package/test has dependency on the tkg test framework and tkgctl cluster client. Please see here: https://github.com/vmware-tanzu/tanzu-framework/blob/main/cmd/cli/plugin/package/test/package_plugin_integration_test.go#L31

https://github.com/vmware-tanzu/tanzu-framework/blob/main/cmd/cli/plugin/package/test/package_plugin_integration_test.go#L33

Now if have only one go.mod under the root cmd/cli/plugin/package, that would mean anyone needing package plugin should also download all the above packages such as tkgcl & test/framework.

What are your thoughts about defining separate go modules; one under cmd/cli/plugin/package and one under cmd/cli/plugin/package/test in order to avoid downloading the packages only required by the test subdirectory by every external component needing only package plugin itself?

@github-actions
Copy link

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

@codegold79 codegold79 marked this pull request as ready for review September 13, 2022 22:48
@codegold79 codegold79 requested review from a team as code owners September 13, 2022 22:48
@github-actions
Copy link

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

@codegold79 codegold79 force-pushed the package-plugin-refactoring-2nd-try branch from 9c9c803 to d9dc37c Compare September 14, 2022 03:52
@codegold79 codegold79 added the ok-to-merge PRs should be labelled with this before merging label Sep 14, 2022
@marckhouzam
Copy link
Contributor

After discussing with @yharish991, we have agreed to pull out the setting of buildinfo for plugins into its own PR.

@codegold79 when #3345 is merged, you will be able to start using github.com/vmware-tanzu/tanzu-framework/cli/runtime/buildinfo instead of github.com/vmware-tanzu/tanzu-framework/pkg/v1/buildinfo.

@codegold79
Copy link
Contributor Author

codegold79 commented Sep 14, 2022

@codegold79 when #3345 is merged, you will be able to start using github.com/vmware-tanzu/tanzu-framework/cli/runtime/buildinfo instead of github.com/vmware-tanzu/tanzu-framework/pkg/v1/buildinfo.

@marckhouzam, that's part of the plan. My first try at refactoring package plugin involved using the relocated buildinfo, but I was advised in a couple of comments that it would be more orderly to use the newly placed buildinfo in separate PRs. So, after this PR #3316 is merged, as long as #3345 is merged, I'll go ahead and use the relocated buildinfo. Hope that makes sense.

UPDATE:
Change in plans. I updated the buildinfo import path in package/plugin/main.go. No need to put in separate PR.

@codegold79 codegold79 requested a review from maralavi September 14, 2022 18:00
Copy link
Contributor

@anujc25 anujc25 left a comment

Choose a reason for hiding this comment

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

Looks good except the go.mod comment from @maralavi

cmd/cli/plugin/package/go.mod Show resolved Hide resolved
cmd/cli/plugin/package/go.mod Outdated Show resolved Hide resolved
@codegold79 codegold79 force-pushed the package-plugin-refactoring-2nd-try branch 7 times, most recently from d274e1f to 2a4b2e6 Compare September 19, 2022 19:17
- bring in openapischema
- go mod init/tidy
- update buildinfo path

Signed-off-by: F. Gold <fgold@vmware.com>
Signed-off-by: F. Gold <fgold@vmware.com>
@codegold79 codegold79 force-pushed the package-plugin-refactoring-2nd-try branch from 2a4b2e6 to 52cc6d3 Compare September 19, 2022 19:29
Copy link
Contributor

@vijaykatam vijaykatam left a comment

Choose a reason for hiding this comment

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

Thanks for the changes

@codegold79 codegold79 merged commit ada7554 into main Sep 20, 2022
@codegold79 codegold79 deleted the package-plugin-refactoring-2nd-try branch September 20, 2022 02:04
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
cla-not-required kind/refactor ok-to-merge PRs should be labelled with this before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants