feat: cast environment variable values to string #218
Conversation
|
Still, wouldn't that be confusing to users? What's the use case in passing non string values to entity which can only host strings? |
|
I'm not sure that it would be all that confusing to users, at least not to me. I've done a fair amount of YAML configuring and I still end up having a mental disconnect at times -- even though I know env values are strings and I am configuring for env, I reliably throw in an int or bool into the yaml config. It doesn't necessarily help that different tools handle value type coercion a bit differently, for example I can see this approach of force-everything-to-be-a-string to be less desirable by some, in which case, I'd be happy to implement an alternative approach which adds validation in function compilation to check that env values are strings and error otherwise; this way it can at least error out early instead of having the error returned via google at deploy time. |
There was a problem hiding this comment.
@edaniszewski thanks for comment.
What handling do you find intutitive if someone puts:
environment:
SOME_ENV_VAR: falseOr introduces an error as:
environment:
dev:
SOME_ENV_VAR: foo
package/lib/compileFunctions.js
Outdated
| _.get(this, 'serverless.service.provider.environment'), | ||
| funcObject.environment // eslint-disable-line comma-dangle | ||
| ), | ||
| (value) => value.toString() |
|
Hmm, thats a good point. For the case of environment:
SOME_ENV_VAR: falseperhaps I was coming at it more from a perspective of convenient and not intuitive, at least for how I'd use it, where having it casted implicitly makes configuring it slightly easier. I think your point on edge cases is a good argument against this approach and more in favor of the approach where no implicit casting is done, but a validation check is run instead. Does that sound okay to you? If so, I'll close out this PR and work on that approach. |
I think it's fine to support coercion just for numbers (there's no ambiguity here I think)and let other types result with same error as it's now. It is fine for me, if you patch it on top of this PR (we anyway squash merge to single commit) |
edaniszewski
force-pushed
the
environment-string-value
branch
from
6e0097e
to
f65c7cf
Compare
|
@medikoo made the changes to coerce numbers to string and error in other cases. not sure if using |
There was a problem hiding this comment.
@edaniszewski thanks for update! I've proposed I believed a cleaner approach. Let me know what you think
package/lib/compileFunctions.js
Outdated
| {}, | ||
| _.get(this, 'serverless.service.provider.environment'), | ||
| funcObject.environment // eslint-disable-line comma-dangle | ||
| funcTemplate.properties.environmentVariables = _.transform( |
There was a problem hiding this comment.
It'll be nicer to use _.mapValues
There was a problem hiding this comment.
yeah, I initially used that approach, but thought that it would be useful to have the key name in the error message as well so the user would know which key was configured incorrectly. I wasn't sure how to easily access keys from mapValues for the error message, so I went with transform.
If including the env var name feels unnecessary, I can update to use mapValues for sure
There was a problem hiding this comment.
In mapValues callback name is provided as second argument, and it's cleaner as you just return coerced value and do not need to overwrite it manually
There was a problem hiding this comment.
ah okay. sorry about my confusion there - I had misread the docs. Thanks for talking me through this. Just pushed an update.
|
Woops. Thats what I get for not running tests before checking in code 🤦 |
|
@medikoo I've updated and fixed the issues causing CI to fail - I think this should be ready to go now. |
This PR:
Related: