-
Notifications
You must be signed in to change notification settings - Fork 187
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
Automate Terraform for oss-vdb-test #1017
Automate Terraform for oss-vdb-test #1017
Conversation
Well, the Github commenting didn't work |
Turns out GitHub Actions coming from public forked repositories can't have more than read permissions, so I can't use an action to make a comment. I'm removing the action, and we'll just have to remember to check the plans. |
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.
thanks! some initial comments.
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 with some minor comments remaining.
@andrewpollock can you please review and sign off on this too?
} | ||
|
||
get-active-account | ||
if [[ (! -z "$active_account") && (! -z "$GCLOUD_SERVICE_KEY") ]]; then |
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.
Nit: I think you might have more parenthesis in here than strictly necessary...
be activated, which will override the account already activated in this container. | ||
|
||
This usually happens if you've defined the GCLOUD_SERVICE_KEY environment variable in a cloudbuild.yaml file & this is | ||
executing in a Google cloud builder environment. |
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.
Nit: "Google Cloud Build" instead?
get-active-account | ||
} | ||
|
||
function service-account-usage() { |
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.
Given this looks more like an error message, emit it on stderr per https://google.github.io/styleguide/shellguide.html#stdout-vs-stderr
function activate-service-key() { | ||
rootdir=/root/.config/gcloud-config | ||
mkdir -p $rootdir | ||
tmpdir=$(mktemp -d "$rootdir/servicekey.XXXXXXXX") |
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.
Looks like you're going with curly braces around variables, which I'm totally cool with, just be consistent: https://google.github.io/styleguide/shellguide.html#variable-expansion
} | ||
|
||
function activate-service-key() { | ||
rootdir=/root/.config/gcloud-config |
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.
Please quote all variables per https://google.github.io/styleguide/shellguide.html#quoting
@@ -0,0 +1,53 @@ | |||
#!/bin/bash |
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.
Please add a file header per https://google.github.io/styleguide/shellguide.html#file-header
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 didn't actually write or edit this file - it came from https://github.com/GoogleCloudPlatform/cloud-builders-community/tree/master/terraform
I mentioned it in the README but I might add a comment in the file (and in Dockerfile and cloud build file) to note this 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.
Oh I'd glossed over that in the README. Treat all of these as nits then... I filed GoogleCloudPlatform/cloud-builders-community#611 against the source.
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'm just going to leave it as-is, so I don't inadvertently break anything.
Adds a number of things needed to start automatically running terraform changes on the staging environment.
(I know Andrew wants to discuss potentially using KCC, but I'm using this to start setting up our staging pipeline)oss-vdb
to runterraform plan
onoss-vdb-test
when terraform files are changedAlso added a Github workflow to comment on PRs when something changes.oss-vdb
to runterraform apply
onoss-vdb-test
on pushes to the master branch.The Cloud Builds use the existing
deployment
service account fromoss-vdb
. IAM permissions have been granted to it onoss-vdb-test
to allow it to modify the project as needed.