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

pinniped-components: Extract GetPinnipedKubeconfig and oFromCluster to shared lib #4533

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

benjaminapetersen
Copy link
Member

Extract GetPinnipedKubeconfig and oFromCluster to shared lib

Continuing with what we began in #4514 this extracts two common functions for interactions with Pinniped and deduplicates them.

What this PR does / why we need it

Which issue(s) this PR fixes

Fixes #

Describe testing done for PR

Release note


Additional information

Special notes for your reviewer

@github-actions
Copy link

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

@joshuatcasey
Copy link
Member

joshuatcasey commented Mar 31, 2023

Love the idea. Let's get the PR checks ✅ and ask for approval.

Do we need the change to the root go.mod?

@benjaminapetersen benjaminapetersen force-pushed the pinniped-components/GetPinnipedKubeconfig-GetPinnipedInfoFromCluster branch from dc7be06 to c9090f8 Compare March 31, 2023 16:36
@github-actions
Copy link

CVE Scan results for this PR can be viewed from
https://github.com/vmware-tanzu/tanzu-framework/security/code-scanning?query=pr%3A4533

@github-advanced-security
Copy link

You have successfully added a new Trivy configuration .github/workflows/cve_scan.yaml:trivy_scan. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4533/20230331164555/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
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4533/20230331164825/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
Copy link

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

@benjaminapetersen
Copy link
Member Author

Think all the replace statements for the multiple mods in this repo are causing the build issue. Will look at this shortly.

Copy link
Contributor

@prkalle prkalle left a comment

Choose a reason for hiding this comment

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

Thank you @benjaminapetersen. It's really great to see this refactoring.
I would wait for CI checks to pass before merging.

@@ -377,6 +377,8 @@ func createServerWithEndpoint() (server *configapi.Server, err error) {
return nil, err
}
} else {
// TODO(BEN): this func has more complicated dependencies, but is obviouisly pinniped related, and it is
Copy link
Contributor

@prkalle prkalle Mar 31, 2023

Choose a reason for hiding this comment

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

After this func is refactored, it would really help core CLI to fetch pinniped kubeconfig by providing the endpoint.
Infact Core CLI would really need this functionality (in pinniped-components/common) where given endpoint to TKG/TKGs cluster library would return the pinniped kubeconfig. So that CLI could fully depend on the library and remove duplicated code.

Copy link
Member Author

Choose a reason for hiding this comment

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

@prkalle if you happen to already have a good eye on all the require() and replace() across this repo and the various modules from you previous work, would love any pointers. I think some of the replace() statements are causing incompatibility issues. Tracking that down next, is breaking the build.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure @benjaminapetersen. We can discuss offline.

@joshuatcasey
Copy link
Member

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants