serverless / serverless-google-cloudfunctions

Serverless Google Cloud Functions Plugin – Adds Google Cloud Functions support to the Serverless Framework
https://www.serverless.com
MIT License
271 stars 127 forks source link

feat: cast environment variable values to string #218

Open edaniszewski opened 4 years ago

edaniszewski commented 4 years ago

This PR:

Related:

medikoo commented 4 years ago

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?

edaniszewski commented 4 years ago

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 docker-compose allows you to configure env values that are strings, ints (which is implicitly cast to string), or null types (which is excluded from env), but not bools.

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.

edaniszewski commented 4 years ago

Hmm, thats a good point. For the case of

environment:
  SOME_ENV_VAR: false

perhaps 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.

medikoo commented 4 years ago

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

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 commented 4 years ago

@medikoo made the changes to coerce numbers to string and error in other cases.

not sure if using _.transform is the best way of doing it, so I'm open to suggestions for other approaches

edaniszewski commented 4 years ago

Woops. Thats what I get for not running tests before checking in code 🤦

edaniszewski commented 4 years ago

@medikoo I've updated and fixed the issues causing CI to fail - I think this should be ready to go now.