google-github-actions / deploy-cloudrun

A GitHub Action for deploying services to Google Cloud Run.
https://cloud.google.com/run
Apache License 2.0
467 stars 116 forks source link

feat: Added support for multiple containers with new parameter 'container' #500

Closed btkelly closed 7 months ago

btkelly commented 7 months ago

This change adds a new, optional parameter called container. This allows the action to specify which container inside the Cloud Run service you are deploying to. By default Cloud Run will just deploy to the first container which is not always desired.

Official command line docs for this argument can be found here.

sethvargo commented 7 months ago

Hi @btkelly - thank you for the PR. Is there a reason you cannot use the --flags input for this? We're trying to avoid adding every single configurable option as inputs, because it becomes quickly unmaintainable.

btkelly commented 7 months ago

@sethvargo great point on the flags, and it totally makes sense for maintainability here. The issue with this particular argument is the order matters and container must come before other flags you want impacted by this. There could be an argument to make this a larger change that would support setting multiple things on multiple containers at once but that would change quite a bit of the parameter structure of the action. I thought this was a simpler change.

I did confirm that setting the container flag last, which would occur with how the action appends the --flags items, throws an error on the command line.

Error: ERROR: (gcloud.run) When --container is specified --image must be specified after --container.

btkelly commented 7 months ago

In further testing, setting the container flag is very picky about the order of arguments. It looks like it also requires all non "container" specific arguments to be before that. I'm going to update this PR with that additional reordering of arguments.

sethvargo commented 7 months ago

Hi @btkelly - thanks, but I'm still concerned here. You can specify the --container flag multiple times, for multiple sidecar containers. For these reasons, I think flags still provides the most flexibility.

btkelly commented 7 months ago

I see what you mean @sethvargo, it does keep it more flexible by just leveraging the --flags parameter. With this in mind, I would recommend a potential README update that indicates how to support multiple containers. Just because the image parameter is no longer required but you are still required to provide the --image flag later on. I can put up a separate PR for README updates if that would be helpful. In the meantime, I'll go the flags route and close this PR. Thanks for taking the time to review 👍