-
Notifications
You must be signed in to change notification settings - Fork 69
Feature: Allow re-uploading subsets of files #1188
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
base: main
Are you sure you want to change the base?
Conversation
5c0f956 to
d76aa84
Compare
98609e5 to
d40289d
Compare
bobheadxi
left a comment
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.
Thank you! just one minor suggested tweak to the docstring
cmd/src/snapshot_upload.go
Outdated
| USAGE | ||
| src snapshot upload -bucket=$BUCKET -credentials=$CREDENTIALS_FILE | ||
| src snapshot upload -bucket=$MIGRATION_BUCKET_NAME -credentials=./migration_private_key.json [-file=$FILE] |
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 think I like the credentials file better when it was $CREDENTIALS_FILE, lets you decide where to put it instead of current working dir where it may be mishandled 👀
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 agree customers should decide where to put it. I'm not clear on the mishandling part.
The CLI doesn't make use of the env vars, and I felt it useful to clarify in the helper text that the -bucket arg is a string, whereas -credentials is a file path.
I've rewritten the helper text to improve it from both angles.
| credentialsPath := flagSet.String("credentials", "", "JSON credentials file for Google Cloud service account") | ||
| trimExtensions := flagSet.Bool("trim-extensions", true, "trim EXTENSION statements from database dumps for import to Google Cloud SQL") | ||
| fileArg := flagSet.String("file", strings.Join(listOfValidFiles, ","), "comma-delimited list of files to upload") | ||
| filterSQL := flagSet.Bool("filter-sql", true, "filter incompatible SQL statements from database dumps for import to Google Cloud SQL") |
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 assume you're updating all docs that exist elsewhere to use renamed flags 🙏
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.
Searching for the trim-extensions flag, it appears to not be documented anywhere else in our code, or in Notion.
The helper text in this repo appears to be the only documentation, sub-command helper text doesn't seem to get printed -- separate issue to resolve.
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 only documentation / reference of this is in our internal process doc, and thus the commands we copy / paste / send to the customer admin, so I'll make the changes to match in this doc once this PR is merged and released.
It happens from time to time that when migrating from self-hosted to Cloud, a customer admin may need to re-upload a database export. These database files can be very large, and may be uploaded from the other side of the planet from the GCS bucket, and some admins may be working from home with slow upload speeds. So, when the IE team needs to ask a customer to re-upload a database file, maybe with a different set of args, or a different format, it can be quite a problem to have to re-upload all 4 files when maybe only 1 is needed.
--filearg, where customer admins can specify all / any / none of the database dump files, and / or the summary.json file, to upload to the GCS bucketTest plan
Tested on marc-dev cloud migration and bucket