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

Update Go Helm v3 to use native client #1296

Merged
merged 3 commits into from
Sep 9, 2020
Merged

Conversation

lblackstone
Copy link
Member

Proposed changes

Related issues (optional)

Part of #920

@lblackstone lblackstone marked this pull request as ready for review September 8, 2020 23:58
@lblackstone
Copy link
Member Author

Note: I changed the tests to test both v2 and v3 and expect no changes between them. I also updated the Chart that is deployed because the previous one is deprecated and will stop working at some point when the k8s cluster version gets bumped.

Copy link
Contributor

@EvanBoyle EvanBoyle left a comment

Choose a reason for hiding this comment

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

Looks good to me. Was a little confused until I found this change that updated the underlying provider implementation (which I thought I was reviewing as a part of this for some reason): #1263

@@ -0,0 +1,312 @@
// Copyright 2016-2020, Pulumi Corporation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason why this file is templated? It doesn't seem to have any substitutions as far as I can tell. Just makes it a little more cumbersome to review. Same goes for the types defined below.

Copy link
Member Author

Choose a reason for hiding this comment

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

IIRC, I did that to keep the Go linter from complaining. I think it was trying to analyze the templates in place and was failing since the paths are wrong unless the files are in the SDK folder. It's not necessary otherwise.

Copy link
Contributor

@metral metral left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Untar *bool `pulumi:"untar"`
Verify *bool `pulumi:"verify"`
Version string `json:"version,omitempty" pulumi:"version"`
CAFile string `json:"ca_file,omitempty" pulumi:"caFile"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious on the json tags: does snake casing here match with what is expected in helm? (as opposed to camelCase or others)

Copy link
Member Author

Choose a reason for hiding this comment

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

This matches the JSON structure expected by the invoked function. It's not used by Helm directly; it's just the way I defined the interface rather than passing a huge bag of args.

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.

3 participants