serverless-google-cloudfunctions icon indicating copy to clipboard operation
serverless-google-cloudfunctions copied to clipboard

feat: cast environment variable values to string

Open edaniszewski opened this issue 4 years ago • 7 comments

This PR:

  • casts the values from environment configuration to strings prior to deploy to prevent GCF invalid argument error
  • adds test cases for various non-string environment values

Related:

  • #128

edaniszewski avatar Jun 23 '20 19:06 edaniszewski

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?

medikoo avatar Jun 24 '20 09:06 medikoo

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 avatar Jun 24 '20 12:06 edaniszewski

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.

edaniszewski avatar Jun 24 '20 13:06 edaniszewski

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)

medikoo avatar Jun 24 '20 13:06 medikoo

@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 avatar Jun 24 '20 18:06 edaniszewski

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

edaniszewski avatar Jun 25 '20 14:06 edaniszewski

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

edaniszewski avatar Jun 30 '20 20:06 edaniszewski