GoogleCloudPlatform / cloud-run-button

Let anyone deploy your GitHub repos to Google Cloud Run with a single click
https://cloud.run
Apache License 2.0
525 stars 91 forks source link

Preserve order of environment variables in app.json -> env #205

Closed mmanciop closed 3 years ago

mmanciop commented 3 years ago

Since the default JSON unmarshalling maps keys in the env object of the app.json file, it is possible that the order of environment variables is not respected, which causes confusion in the end user when more than one value needs to be prompted. (While technically JSON keys in an object are supposed to be order insensitive, inconsistency in the end-user steps are nevertheless an issue.)

This change introduces custom unmarshalling of app.json -> env to preserve the order of prompted environment variables as specified in the app.json file.

mmanciop commented 3 years ago

The build error seems entirely unrelated with this PR. Anybody to retrigger?

jamesward commented 3 years ago

Thanks for putting this together! It seems like there are cases where for a good user experience, ordering the questions is important. However, I'm not sure if it is a good idea to do this with a custom unmarshaller. An alternative would be to add an optional priority field. Will let @ahmetb chime in on that.

It looks like the GitHub action just had a temporary issue. I've restarted it.

ahmetb commented 3 years ago

Hey, thanks for the answer! It's mostly my fault there's been radio silence here.

I agree with the problem, though I think your current solution which involves writing a custom Unmarshaler seems to a bit lengthier than I expected and might be a pain to maintain. (I can guarantee you we’ll forget to refactor UnmarshalJSON() when we add/change something in the env type.)

Also, there's a reason nearly no implementation tries to maintain map ordering from JSON, because it's not meant to be preserved. Software like Kubernetes solve this by making "envs" keys a "list", rather than a "dict", e.g.:

envs:
- key: X
  value: Y
- [...]

Here’s what I will recommend, to see if you and @jamesward can agree:

  1. We should make env field an array (list) sooner than later.
  2. We should do (1) in a non-breaking way (read: support both at the same time)
  3. To do (2), we can create a new type called EnvSliceOrMap that has a slice and map value.
  4. To parse a .json file into slice or map, we can parse it into an anonymous struct such as map[string]interface{}, then we can see if v["env"] (if exists) is a []interface{} (meaning slice i.e. v2 format) or a map[string]interface{} (meaning a map, i.e. the v1 format).
  5. Then we populate EnvSliceOrMap.{slice,map} fields, and convert one to another internally (through a method while using it in the rest of the program.

If agreeable, let's pursue that. I can also implement this myself if you’d prefer that.

Also let's not forget that in this new model, we might need to define what happens if multiple env variables exist in the env list with the same name.

ahmetb commented 3 years ago

I’ll go with a simpler implementation proposed by @jamesward above. We’ll introduce an integer field to env map, signaling to the prompts an ordering of the listed env vars.

Thanks for the implementation @mmanciop. Next time, just feel free to propose the change without code.