josmo / drone-ecs

Drone plugin for triggering Amazon EC2 Container Service (ECS) deployments
Apache License 2.0
30 stars 41 forks source link

Update network-mode env var to be consistent with other CLI flags. #33

Closed jrasell closed 2 years ago

jrasell commented 5 years ago

The network-mode CLI flag had an env var set to PLUGIN_TASK_NETWORK_MODE which broken convention with all other CLI flags and made it confusing to switch between using drone.yaml config files and running the plugin differently.

This change updates the env var to be consistent so the user experience is smoother.

josmo commented 5 years ago

Isn't the network mode for the task as a whole and not the container? ie there's cpu container and cpu task env vars. I'm down to change this to not be specific to task but I just want to make sure it would make sense :) if it does we should still keep the old env var with coma separated so that it doesn't break for folks using it :)

jrasell commented 5 years ago

@josmo the network mode is related to the task def. as you mention, therefore it's probably correct to keep the envvar as original and update the CLI flag. With this, would you prefer the current flag is updated or a second added to preserve backwards compatibility with both values checked and the original flag taking preference?

josmo commented 5 years ago

Are you meaning updating flag name to task-network-mode instead of network-mode ? in that case there wouldn't be any breaking changes since that would only impact the internals, the ENV vars would still be the same :) if that's what you mean then go for it :)

jrasell commented 5 years ago

thanks for your time @josmo, I have pushed the discussed updates.

josmo commented 5 years ago

Thanks @jrasell you'll also need to change c.String("network-mode") to be c.String("task-network-mode") for the variable to get injected in :) it's around line 225 in main

josmo commented 2 years ago

This is been outstanding for a while, Feel free to reopen against main with the changes and I'll merge it in.