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

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

Open
benjaminapetersen wants to merge 5 commits into
mainfrom
pinniped-components/GetPinnipedKubeconfig-GetPinnipedInfoFromCluster
Open

pinniped-components: Extract GetPinnipedKubeconfig and oFromCluster to shared lib#4533
benjaminapetersen wants to merge 5 commits into
mainfrom
pinniped-components/GetPinnipedKubeconfig-GetPinnipedInfoFromCluster

Conversation

@benjaminapetersen
Copy link
Copy Markdown
Contributor

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

@benjaminapetersen benjaminapetersen requested a review from a team as a code owner March 29, 2023 21:28
@benjaminapetersen benjaminapetersen requested a review from a team March 29, 2023 21:28
@benjaminapetersen benjaminapetersen requested a review from a team as a code owner March 29, 2023 21:28
@github-actions
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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
Copy Markdown
Contributor 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
Copy Markdown
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.

return nil, err
}
} else {
// TODO(BEN): this func has more complicated dependencies, but is obviouisly pinniped related, and it is
Copy link
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
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
Copy Markdown
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.

5 participants