Update NodeJS Helm v3 to use native client #1279
Conversation
lblackstone
force-pushed
the
lblackstone/helm-ts-sdk
branch
4 times, most recently
from
3408c3e to
b0028b5
Compare
lblackstone
force-pushed
the
lblackstone/helm-ts-sdk
branch
from
b0028b5 to
b5b07af
Compare
| ...config, | ||
| releaseName, | ||
|
|
||
| toJSON() { |
There was a problem hiding this comment.
Not sure if there's a simpler way to handle the translation from the input interfaces to the JSON string, but I had to add cases for all of the camelCase fields.
There was a problem hiding this comment.
I had to add cases for all of the camelCase fields.
camelCase and snake_case are both conventions that are valid JSON.
What is the need for the translation?
Would we have to manually support these entries for future options, or can we avoid that all together?
Also, TIL about toJSON(). The embedding of logic on the config props like this is stylistically new to me.
There was a problem hiding this comment.
The invoke call is expecting a JSON string that it unmarshals to the Helm structs in the provider, so I need the keys to match. I'm not sure if there's a better way to handle this, but the data was complex enough that I thought JSON was nicer than using individual fields.
There was a problem hiding this comment.
Also, TIL about toJSON(). The embedding of logic on the config props like this is stylistically new to me.
Courtesy of https://www.executeprogram.com/courses/modern-javascript/lessons/customizing-json-serialization
|
|
||
| const allConfig = pulumi.output(config); | ||
|
|
||
| (<any>allConfig).isKnown.then((isKnown: boolean) => { |
There was a problem hiding this comment.
Since allConfig is an output, why not use it in an apply like is done below with this.resources?
Relatedly, using <any> is loose on typing, can we preserve it by using it within the apply?
There was a problem hiding this comment.
Hmm, I'm not sure why it's like this. I copy-pasted this code from the original implementation. I'll leave it alone for this PR for the sake of not accidentally breaking something.
| ...config, | ||
| releaseName, | ||
|
|
||
| toJSON() { |
There was a problem hiding this comment.
I had to add cases for all of the camelCase fields.
camelCase and snake_case are both conventions that are valid JSON.
What is the need for the translation?
Would we have to manually support these entries for future options, or can we avoid that all together?
Also, TIL about toJSON(). The embedding of logic on the config props like this is stylistically new to me.
| } | ||
| } | ||
|
|
||
| interface BaseChartOpts { |
There was a problem hiding this comment.
Are BaseChartOpts and ChartOpts interfaces that could exist at the top of the Helm namespace given that the options are shared, or is it best to keep these versioned as we've done?
apiVersions is new, so that's prob a case to keep them versioned.
There was a problem hiding this comment.
They could probably be shared, but I think I'll leave them separate in case we want to diverge in the future given the upcoming deprecation plan for v2.
lblackstone
force-pushed
the
lblackstone/helm-ts-sdk
branch
from
9ec4593 to
b92c6a5
Compare
lblackstone
force-pushed
the
lblackstone/helm-ts-sdk
branch
from
eedee40 to
c7fa145
Compare
| @@ -1,3 +1,4 @@ | |||
| apiVersion: v2 | |||
There was a problem hiding this comment.
Apparently this is correct. v1 corresponds to Helm v2, and v2 corresponds to Helm v3. 🤷♂️
Proposed changes
Related issues (optional)
Part of #920