Skip to content
This repository has been archived by the owner on Sep 3, 2022. It is now read-only.

Cleanup backup tar after copying to GCS #1152

Merged

Conversation

yebrahim
Copy link
Contributor

Also, try using project_id as bucket name if other options have failed.

@@ -123,7 +123,11 @@ fi
# test and create bucket if necessary
gsutil ls gs://${gcs_bucket} &>/dev/null || {
echo "Bucket '${gcs_bucket}' was not found. Creating it.."
gsutil mb gs://"${gcs_bucket}"
gsutil mb gs://"${gcs_bucket}" || {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest a slightly different ordering of the logic where we only try to run gsutil mb once:

Basically, we'd look at the "gcs_bucket" (without any default), and "project_id" environment variables to figure out what the bucket name should be.

If "gcs_bucket" is defined, then that is the bucket name. Otherwise we run gsutil ls gs://${project_id}.appspot.com to see if that bucket exists, and if so set the bucket name to it. Finally, if neither of the two previous tests passed, then we set the bucket name to match the project ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean we shouldn't create the gs://${project_id}.appspot.com bucket if it doesn't exist? Other than this, I don't see much difference between the comment and the changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to not trying to create the "...appspot.com" bucket (which we know would always fail), the other difference between what I proposed and the current approach is that we would not try to create the project bucket if the user has explicitly specified a gcs_bucket, regardless of whether or not that gcs_bucket exists.

@yebrahim yebrahim force-pushed the yebrahim/cleanup-gcsbackup-tar branch from fc3811b to 5685f70 Compare January 27, 2017 01:47
@yebrahim yebrahim force-pushed the yebrahim/cleanup-gcsbackup-tar branch from 5685f70 to cb5de7c Compare January 27, 2017 01:50
else
gsutil ls gs://"${gcs_bucket}" &>/dev/null || {
echo "Could not list bucket '${gcs_bucket}'. Will try to create it.."
gsutil mb gs://"${gcs_bucket}" || true
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have an "|| true"?

If this mb call fails, then the script should exit.

@yebrahim yebrahim merged commit 690b9f4 into googledatalab:master Jan 27, 2017
@yebrahim yebrahim deleted the yebrahim/cleanup-gcsbackup-tar branch January 27, 2017 04:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants