Convert param input to String

[tigris]

The ruby AWS SDK always expects string input. Even if your cloudformation parameter type is Number, it still wants it to be a ruby String. The validation of parameter value will happen in cloudformation land.

Currently stackup is just passing through the raw YAML value to the SDK as is - which means it is up to the caller to understand the nuances on the ruby AWS SDK (where everything must be a string). Given stackup is supposed to "help" reduce cognitive load, I think it's ok for it to help convert our YAML values in this instance.

[djmattyg007]

Hi Dan! 🙂 I hope you're doing well ❤️

I feel like this change makes sense, but I'm curious about a couple of things:

1. What happens if someone passes a list of strings or a list of integers? I'm not familiar with how Ruby casts arrays and hashes to strings.
2. What should happen if someone passes nil for a parameter value? Right now it looks Ruby will convert it to an empty string, but perhaps the parameter should just be removed?
3. I think Ruby converts booleans to a lower-case string with either "true" or "false", so I guess they will work sanely with these changes. Should we add a test for it though?

[rea-jonpad]

1. What happens if someone passes a list of strings or a list of integers? I'm not familiar with how Ruby casts arrays and hashes to strings.

The methods affected to_a and to_hash don't directly take arguments. Instead they're provided when the object is initialized.

The expected arguments seem to be a list of hashes, where each item in the hashes are strict key / value pairs, and I don't believe it supports the values being anything other than a simple string.

Dan's latest commit sanitizes the values when initializing the object, rather than when outputting, which I think is the better approach.

[rea-jonpad]

LGTM 👍

[mdub]

Makes sense; Parameter values are supposed to be strings.

Also, hi!

[tigris]

What happens if someone passes a list of strings or a list of integers? I'm not familiar with how Ruby casts arrays and hashes to strings.

Looking at the docs, it seems it's not possible to pass an array or hash/dict/object as a param. The stackup library is not currently validating this at all and blindly slurps in the yaml object and passes it as the parsed object. Which of course fails, for the same reason the Integer fails (cloudformation wants a string). Anyone attempting to do this would be receiving failures.

What should happen if someone passes nil for a parameter value? Right now it looks Ruby will convert it to an empty string, but perhaps the parameter should just be removed?

This would be a null in YAML, and yes the .to_s I've added would convert that to empty string. I can't imagine many people are using param_key: null in their YAML out in the wild. If you do, you get this error: ERROR: Invalid input for parameter key MinInstances. Need to specify either usePreviousValue as true or a value for the parameter -- whereas with the empty string change you will instead get ERROR: Parameter 'MinInstances' must be a number.

I guess this would be a breaking change in a way -- previous instances of folks using null would not deploy, it would break. But forcing it to string would actually deploy something if the param was Type: String in the CF and this is perhaps not what was intended... (Only way I see this happening is if the params file is generated via tooling and put a null in there for reasons)

I think Ruby converts booleans to a lower-case string with either "true" or "false", so I guess they will work sanely with these changes. Should we add a test for it though?

This is correct. A YAML true becomes a ruby <Class>True which stringifies to "true". Currently if you're using YAML true, it won't work. This change makes it work.

[tigris]

Perhaps to alleviate the chance of breaking changes, I can update this change to only impact Integer and Boolean values.

[djmattyg007]

What do people think about the following ideas?

• For nil, exclude the parameter altogether. Don't pass it to CloudFormation.
• For arrays, convert it to a comma-separated string. If your parameter is typed as a list, CloudFormation expects it as a CSV.

[tigris]

For arrays, convert it to a comma-separated string. If your parameter is typed as a list, CloudFormation expects it as a CSV.

Sure. I've done this now.

For nil, exclude the parameter altogether. Don't pass it to CloudFormation.

I feel less ok about this, i'm totally not sure who's stuff out there would break. Not mine, and likely not anyone, but it's also likely no one is setting nil so probably not worth the risk of impact. Plus it's a bit annoying to implement, since we're saying the existence of a key is defined by it's value - where the current code implementation treats keys and values agnostically.

[djmattyg007]

@tigris I just wanted to make sure you knew that we haven't forgtten about this. I've been on leave but will be back on deck later this week 👍

[djmattyg007]

Oops. I forgot about this after getting back from my leave :( I'm taking another look now.

[djmattyg007]

I'm curious why this is a class. Couldn't it just be a plain function?

[tigris]

I'm curious why this is a class. Couldn't it just be a plain function?

Was following the convention used elsewhere in the same file, e.g. ParameterStruct

[djmattyg007]

Thanks for the contribution Dan, and sorry it took me so long to get to it :(

I'll see what's required to issue a new release later today 👍