-
-
Notifications
You must be signed in to change notification settings - Fork 289
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updated sealed secret app to new style #257
Updated sealed secret app to new style #257
Conversation
Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide. |
b1a6996
to
1da711d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry this will require a rebase due to #258 being merged. @Waterdrips what does @nitishkumar71 need to do to rebase this? |
yep, delete the section at the top with kubeConfigPath, _ := command.Flags().GetString("kubeconfig") Then call options.WithKubeconfigPath(kubeConfigPath) into the installer options section. This will fix the issue with not using a supplied kubeconfig flag |
18e5d82
to
6777f93
Compare
@Waterdrips I have made the changes. please review the changes. |
cmd/apps/sealed_secret_app.go
Outdated
@@ -36,7 +38,6 @@ func MakeInstallSealedSecrets() *cobra.Command { | |||
if err := config.SetKubeconfig(kubeConfigPath); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we dont need to set it here, its gets set in the chart app
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for mention. I have made the changes. @Waterdrips
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment on last review, no need to set kubeconfig -its done in the chart app
6777f93
to
48cf185
Compare
@Waterdrips ready for another review. |
cmd/apps/sealed_secret_app.go
Outdated
WithNamespace(namespace). | ||
WithHelmPath(path.Join(userPath, ".helm")). | ||
WithHelmRepo("stable/sealed-secrets"). | ||
WithHelmURL("https://charts.helm.sh/stable"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bitnami should have a new home for the chart. Can you look around and see if you can find it? The "stable helm repo" is going to be deprecated shortly.
FYI https://twitter.com/HelmPack/status/1322224008822202368?s=20 I've also checked the Bitnami Charts repo (nothing there yet) and asked on their |
I found out here -> bitnami-labs/sealed-secrets#389 Thanks @mkmik |
@alexellis Thanks for pointing it out. I think, we should wait for the new chart repo of sealed secrets. What do you suggest? |
No, just go ahead. We already have this pointing at the stable repo. |
Signed-off-by: Nitishkumar Singh <[email protected]> updated code based on alexellis#258 Signed-off-by: Nitishkumar Singh <[email protected]> removed additional line Signed-off-by: Nitishkumar Singh <[email protected]> removed unwanted line Signed-off-by: Nitishkumar Singh <[email protected]> removed unwanted code Signed-off-by: Nitishkumar Singh <[email protected]> corrected repo path Signed-off-by: Nitishkumar Singh <[email protected]>
48cf185
to
d8a0c68
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved
In retrospect and due to #274 I think we do need to switch over to the new helm repo, but it should be a one line change if you can work on that today @nitishkumar71 ? My understanding is that the charts have now moved to |
Description
Motivation and Context
Convert Sealed Secrets App to use "new" pkg/apps/helm_app style #256
[App] Sealed Secrets #220
How Has This Been Tested?
arakade install sealed-secrets
Types of changes
Checklist:
git commit -s