Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try to find project_id in env vars. #156

Closed
wants to merge 1 commit into from

Conversation

simi
Copy link

@simi simi commented Mar 23, 2022

Recently I have found out (during migration to gcloud-setup + auth combination from gcloud-setup deprecated action usage) that even we have set GCP_PROJECT env variable in our GitHub Actions workflow setup, it is ignored and (mainly) it is replaced by first project at the related account which resulted into pushing docker container in different project at GCP.

I would expect if those variables are set to not override them when they are already present.

Also in general it would be good to update README with list of env variables updated by this action.

@simi simi requested a review from a team as a code owner March 23, 2022 18:00
@sethvargo
Copy link
Member

I'm happy to update the README to list the environment variables that are set, but I'm not in favor of this change. The action should only set the project ID from the project_id value or as extracted from the given credential. I think it would be very unexpected for the value to be inherited from an existing environment variable.

@simi
Copy link
Author

simi commented Mar 23, 2022

The problem is it works now differently than before. gcloud-setup (which was enough before on its own) doesn't set GCP_PROJECT (or other variables mentioned). Also I don't expect action to silently update my env variables.

What about to at least raise a warning when env var is being updated when already present?

@sethvargo
Copy link
Member

setup-gcloud was specifically focused on installing and configuring the gcloud CLI, so it did not configure environment variables that other tools and frameworks consume. auth is a general purpose GitHub Action for authenticating future steps to GCP. These steps could be gcloud, but they could also be third-party tooling like Terraform, client SDKs, or bash scripts.

Also I don't expect action to silently update my env variables.

We should definitely document this. We can also add a configuration option to not export any of these:

uses: auth
with:
  # ...
  export_environment: false

What about to at least raise a warning when env var is being updated when already present?

Also a good idea.

I can take on both of these contributions if you want.

@simi
Copy link
Author

simi commented Mar 23, 2022

setup-gcloud was specifically focused on installing and configuring the gcloud CLI, so it did not configure environment variables that other tools and frameworks consume. auth is a general purpose GitHub Action for authenticating future steps to GCP. These steps could be gcloud, but they could also be third-party tooling like Terraform, client SDKs, or bash scripts.

clear 👍

Also I don't expect action to silently update my env variables.

We should definitely document this. We can also add a configuration option to not export any of these:

uses: auth
with:
  # ...
  export_environment: false

This would work (prevent the problem I have faced) only when this will be disabled by default. But if I understand it well, your idea is to just make this opt-out. But combined with warning explaining you can disable the export using this opt-out method would be nice. You'll get notified about the problem and get all the info what's happening making it easy to decide what's the best solution (disable export or rename your env variables to not be in conflict with those set by this action).

I can take on both of these contributions if you want.

Feel free to. Also feel free to close this PR. I was just about to share my experience of silently rewritten env variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants