Skip to content
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

Support configuration of chartPathOptions in install/upgrade client interfaces #181

Open
vl306 opened this issue Aug 12, 2023 · 2 comments

Comments

@vl306
Copy link

vl306 commented Aug 12, 2023

The existing helm install/upgrade client interface functions doesn't seem to support configuration of the chartPathOptions. We have a use case where we want to set "client.ChartPathOptions.InsecureSkipTLSverify = true".

One option is to modify the functions to take the additional argument.

If this the right/preferred approach?.

Existing Client interface:

 InstallOrUpgradeChart(ctx context.Context, spec *ChartSpec, opts *GenericHelmOptions) (*release.Release, error)
 InstallChart(ctx context.Context, spec *ChartSpec, opts *GenericHelmOptions) (*release.Release, error)
 UpgradeChart(ctx context.Context, spec *ChartSpec, opts *GenericHelmOptions) (*release.Release, error)

Proposed Changes:

  InstallOrUpgradeChart(ctx context.Context, spec *ChartSpec, opts *GenericHelmOptions, chartPathOptions *action.ChartPathOptions) (*release.Release, error)
  InstallChart(ctx context.Context, spec *ChartSpec, opts *GenericHelmOptions, chartPathOptions *action.ChartPathOptions) (*release.Release, error)
  UpgradeChart(ctx context.Context, spec *ChartSpec, opts *GenericHelmOptions, chartPathOptions *action.ChartPathOptions) (*release.Release, error)

Alternatively, to avoid changing the interface, the ChartPathOptions can be added to GenericHelmOptions:

@@ -88,8 +88,9 @@ type HelmClient struct {
 }

 type GenericHelmOptions struct {
-       PostRenderer postrender.PostRenderer
-       RollBack     RollBack
+       PostRenderer     postrender.PostRenderer
+       RollBack         RollBack
+       ChartPathOptions *action.ChartPathOptions
 }
@andreas-kupries
Copy link

Agreed, and yes please.

My case is not about skipping TLS verification, albeit related.
Instead of disabling verification I wish to provide the necessary supporting certificates through the ChartPathOptions.CaFile field.

@vl306
Copy link
Author

vl306 commented Oct 12, 2023

The following pull request contains the suggested changes. Just waiting for someone with permissions to merge the pull request:

#182

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

No branches or pull requests

2 participants