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

Add support for specifying credentials #35

Merged
merged 3 commits into from
Apr 2, 2020

Conversation

ianks
Copy link
Contributor

@ianks ianks commented Apr 19, 2019

This PR allows the credentials option to be specified for Storage. This way, we do not have to use global credentials.

@DanielASAndrews
Copy link

@ianks All for this

But I think you forgot to pass the opts object through to the storage initialization ?

@ianks
Copy link
Contributor Author

ianks commented Mar 31, 2020

Finally, fixed. @DanielASAndrews @renchap

Copy link
Owner

@renchap renchap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, and sorry for the time it took me to look at it.

I added 2 comments about your changes in the test environment creation script, can you either address them or remove those changes from this PR?

@@ -73,7 +79,7 @@ gcloud beta billing projects link $GOOGLE_CLOUD_PROJECT --billing-account=$GOOGL

gcloud iam service-accounts create $GCS_SA \
--project=$GOOGLE_CLOUD_PROJECT \
--display-name $GCS_SA
--display-name $GCS_SA || true
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to not use || true here but check for the specific error / error code.
If not possible, I would just abort, the user can do it manually if she has a special case

@@ -58,7 +58,13 @@ fi
# Display every command executed from now
set -x

gcloud projects create $GOOGLE_CLOUD_PROJECT
gcloud projects create $GOOGLE_CLOUD_PROJECT || read -p "Looks like this service accounts exists. If so, continue? (y/n)? " CONT
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this command return an error when a service account already exists, or the project? I am not really keen to add complexity in this script and handle the various cases, it is meant as a simple helper to set up the test environment + documentation on how to do so manually if needed.

@ianks
Copy link
Contributor Author

ianks commented Apr 2, 2020

I just removed the diff @renchap

@ianks ianks requested a review from renchap April 2, 2020 03:32
@renchap renchap merged commit 5254d17 into renchap:master Apr 2, 2020
@renchap
Copy link
Owner

renchap commented Apr 2, 2020

Thanks! I will release 3.0.1.

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

Successfully merging this pull request may close these issues.

3 participants