getporter / helm2-mixin

Helm mixin for Porter
https://porter.sh/mixins/helm
Apache License 2.0
13 stars 7 forks source link

feat(*): support non-string set values #28

Closed vdice closed 5 years ago

vdice commented 5 years ago

This PR adds support for generic-type values to be supplied on corresponding helm {install,upgrade} ... --set key=value commands.

Here I use interface{} as the type for these values, but unsure if this is too generic?

Discovered this deficiency when installing the azure-mysql-wordpress example bundle, wherein some values intend to be set as integers (externalDatabase.port: 3306, etc.)

Not sure if my json schema amendments are correct 😉

vdice commented 5 years ago

Thanks @carolynvs . Let's start from the issue/behavior I was seeing and then we can work on the right solution.

With the latest releases of porter and mixins, when installing the azure-wordpress-mysql example bundle, on the Helm Install Wordpress step, installation fails with:

Helm Install Wordpress
err: install.0.helm.set: Invalid type. Expected: string, given: integer
    * install.0.helm.set: Invalid type. Expected: string, given: integer
Error: install.0.helm.set: Invalid type. Expected: string, given: integer
    * install.0.helm.set: Invalid type. Expected: string, given: integer
Error: mixin execution failed: exit status 1
Error: failed to install the bundle: container exit code: 1

Since this bundle had previously successfully installed, I realize now that the cause of this is the updated/added mixin schema, right? I believe it now deems any non-string value to be passed in, as in this line of the example porter.yaml, invalid.

So, perhaps the only change we need to make is to adjust the schema to allow for set values of types other than string? Perhaps as long as we also allow for integer and boolean, we should be good?

carolynvs commented 5 years ago

Yeah I'm guessing that we could be fine with just changing the schema to additionalProperties: true instead of restricting them to strings and it would work? Worth a shot.

vdice commented 5 years ago

Good call @carolynvs ! Closing in favor of https://github.com/deislabs/porter-helm/pull/30