-
Notifications
You must be signed in to change notification settings - Fork 545
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
Implement Billing Account Role #53
Conversation
added more output so that the user knows that execution is happening
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, @adrienthebo to review as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified that the script runs as expected with and without a billing account, and the changes are reasonable. LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a small error where we don't clean up the billing account policy file, could you fix that before we merge this?
helpers/setup-sa.sh
Outdated
echo "Could not set roles/billing.user on service account $SERVICE_ACCOUNT.\ | ||
Please perform this manually." | ||
fi | ||
rm -f policy-tmp-$$.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extension of this file is incorrect, it should be .yml
but is .json
.
helpers/setup-sa.sh
Outdated
\ \ role: roles/billing.user" policy-tmp-$$.yml | ||
gcloud beta billing accounts set-iam-policy $BILLING_ACCOUNT policy-tmp-$$.yml | ||
else | ||
echo "Could not set roles/billing.user on service account $SERVICE_ACCOUNT.\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small fixup: since we used a 2 space indent in the previous code, could we switch this block to use 2 spaces as well?
Fixed both requested changes @adrienthebo |
This adds to the helper script the optional creation of the IAM billing role for the service account that is required. Other small changes include: