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

Add support for using values from yaml files #1828

Merged
merged 6 commits into from
Dec 9, 2021
Merged

Conversation

viveklak
Copy link
Contributor

@viveklak viveklak commented Dec 9, 2021

Proposed changes

Related issues (optional)

Fixes #1700

@github-actions
Copy link

github-actions bot commented Dec 9, 2021

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

1 similar comment
@github-actions
Copy link

github-actions bot commented Dec 9, 2021

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@github-actions
Copy link

github-actions bot commented Dec 9, 2021

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

if err := mapstructure.Decode(stripped, &release); err != nil {
return nil, fmt.Errorf("decoding failure: %w", err)
}
release.Values = mergeMaps(release.Values, values)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The precedence - if it's not clear - will prioritize the values in the values field over what is specified in the files.

Copy link
Member

@lblackstone lblackstone left a comment

Choose a reason for hiding this comment

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

Changes LGTM, but you'll also need to update the description for the valueYamlFiles parameter in the schema.

return nil, err
}
default:
return nil, fmt.Errorf("unsupported type for 'valueYamlFiles' arg: %T", v)
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to also support Archives? I just noticed that the SDK type supports both Assets and Archives.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to keep this in-line with the helm CLI. The CLI takes yaml files and not archives. We can reconsider if we get requests from users. I can't think of an obvious use case where archives would be preferred.

@viveklak
Copy link
Contributor Author

viveklak commented Dec 9, 2021

Changes LGTM, but you'll also need to update the description for the valueYamlFiles parameter in the schema.

Good catch! Updated now.

@github-actions
Copy link

github-actions bot commented Dec 9, 2021

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@viveklak viveklak merged commit 5e0e953 into master Dec 9, 2021
@pulumi-bot pulumi-bot deleted the vl/ValuesAsset branch December 9, 2021 22:07
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

Successfully merging this pull request may close these issues.

[Helm/Release] Load values from yaml files in addition to inline specified values
2 participants