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.

Sign up for GitHub

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

Jump to bottom

Fix HELM_HOME handling for v3 #1076

Merged
merged 7 commits into from
Apr 22, 2020
Merged

Fix HELM_HOME handling for v3 #1076

merged 7 commits into from
Apr 22, 2020

Conversation

lblackstone
Copy link
Member

@lblackstone lblackstone commented Apr 17, 2020 •
edited

Proposed changes

Helm v3 removed the --home parameter, but still supports the HELM_HOME env var for backward compatibility. Pass ambient env vars to the helm binary so this works properly.

TODO:

  • NodeJS
  • Python
  • C#

Related issues (optional)

Fix #1070

@lblackstone lblackstone marked this pull request as ready for review April 17, 2020 22:41
mikhailshilkov
mikhailshilkov reviewed Apr 20, 2020
View reviewed changes
sdk/dotnet/Helm/ChartBase.cs Outdated Show resolved Hide resolved
sdk/dotnet/Helm/ChartBase.cs Outdated Show resolved Hide resolved
sdk/dotnet/Helm/ChartBase.cs Outdated Show resolved Hide resolved
sdk/dotnet/Utilities.cs Outdated Show resolved Hide resolved
sdk/nodejs/helm/v2/helm.ts
if (opts.devel === true) { flags.push(`--devel`); }
if (opts.prov === true) { flags.push(`--prov`); }
if (opts.verify === true) { flags.push(`--verify`); }
}
execSync(`helm fetch ${shell.quote([chart])} ${flags.join(" ")}`);
execSync(`helm fetch ${shell.quote([chart])} ${flags.join(" ")}`, { env });
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we want to pass all env variables as opposed to passing HELM_HOME specifically? Is there a combination of env variables that would change the behavior of helm between two Pulumi versions, and therefore break somebody?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose it's possible. The advantage of passing everything is that it makes it easier for users to configure Helm behavior without requiring us to change code, but maybe it would be better to limit the change here.

Copy link
Member

Choose a reason for hiding this comment

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

The default for execSync was to pass process.env down to the subprocess. So I think we should keep that behaviour by default (and add HELM_HOME).

Copy link
Member

Choose a reason for hiding this comment

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

Indeed! The same applies to .NET, actually, if I read the docs correctly. Therefore, we can simplify the PR and not retrieve/pass env variables, only add HELM_HOME if needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lukehoban This comment indicates that setting env only sets the passed values, which is why I copied process.env explicitly.

Copy link
Member

Choose a reason for hiding this comment

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

@lblackstone That makes sense to me. I think we don't need to retrieve/pass them for .NET - just add the extra one.

sdk/nodejs/helm/v2/helm.ts Show resolved Hide resolved
sdk/nodejs/helm/v2/helm.ts Show resolved Hide resolved
@lblackstone
Copy link
Member Author

@mikhailshilkov Any idea on this error?

[              helm-api-versions ]   kubernetes:helm.sh:Chart (api-versions):
[              helm-api-versions ]     error: Pulumi.ResourceException: Value does not fall within the expected range.
[              helm-api-versions ]        at Pulumi.Kubernetes.Helm.ChartBase.<>c__DisplayClass0_0.<.ctor>b__1(ValueTuple`2 values)
[              helm-api-versions ]        at Pulumi.Output`1.ApplyHelperAsync[U](Task`1 dataTask, Func`2 func)
[              helm-api-versions ]        at Pulumi.Output`1.Pulumi.IOutput.GetDataAsync()
[              helm-api-versions ]        at Pulumi.Serialization.Serializer.SerializeAsync(String ctx, Object prop)
[              helm-api-versions ]        at Pulumi.Deployment.SerializeFilteredPropertiesAsync(String label, IDictionary`2 args, Predicate`1 acceptKey)
[              helm-api-versions ]        at Pulumi.Deployment.SerializeAllPropertiesAsync(String label, IDictionary`2 args)
[              helm-api-versions ]        at Pulumi.Deployment.RegisterResourceOutputsAsync(Resource resource, Output`1 outputs)
[              helm-api-versions ]        at Pulumi.Deployment.Runner.WhileRunningAsync()

lukehoban
lukehoban reviewed Apr 21, 2020
View reviewed changes
provider/pkg/gen/nodejs-templates/helm/v2/helm.ts
const yamlStream = execSync(
cmd,
{
env: {...process.env},
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this the default already?

sdk/nodejs/helm/v2/helm.ts
if (opts.devel === true) { flags.push(`--devel`); }
if (opts.prov === true) { flags.push(`--prov`); }
if (opts.verify === true) { flags.push(`--verify`); }
}
execSync(`helm fetch ${shell.quote([chart])} ${flags.join(" ")}`);
execSync(`helm fetch ${shell.quote([chart])} ${flags.join(" ")}`, { env });
Copy link
Member

Choose a reason for hiding this comment

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

The default for execSync was to pass process.env down to the subprocess. So I think we should keep that behaviour by default (and add HELM_HOME).

@mikhailshilkov
Copy link
Member

@lblackstone

Any idea on this error?

Hmm, I suspect we screwed up environment variables related to paths somehows. I think we should revert to not passing all the env variables explicitly and only pass HELM_HOME in addition to the ambient defaults - and I think the error will be gone.

lukehoban
lukehoban approved these changes Apr 21, 2020
View reviewed changes
Copy link
Member

@lukehoban lukehoban left a comment

Choose a reason for hiding this comment

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

Do we have any way to test this?

The changes look good to me - but there is enough subtlety here that it would be great if we could lock this in with test coverage.

@lblackstone
Copy link
Member Author

Do we have any way to test this?

I specified the home option on a test Chart for each SDK. This should be a no-op if things are working, and fail in current master if the helm v3 binary is used.

@lblackstone lblackstone merged commit e83039f into master Apr 22, 2020
@pulumi-bot pulumi-bot deleted the lblackstone/fix-helm-home branch April 22, 2020 14:42
@renovate renovate bot mentioned this pull request Jun 6, 2021
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukehoban lukehoban lukehoban approved these changes

@mikhailshilkov mikhailshilkov Awaiting requested review from mikhailshilkov

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Presence of HELM_HOME environment variable causes Helm v3 command to fail
3 participants
@lblackstone @mikhailshilkov @lukehoban