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

no-check option for asset uploading, default for release #3304

Merged
merged 6 commits into from
Oct 6, 2020

Conversation

maiamcc
Copy link
Contributor

@maiamcc maiamcc commented May 7, 2020

Hello @nicks, @jazzdan,

Please review the following commits I made in branch upload-assets-messaging:

6d5a606 (2020-05-07 18:24:25 -0400)
no-check option for asset uploading, default for release

e6e313b (2020-05-07 17:34:35 -0400)
scripts: upload-assets.py tells you what to do in case of conflict

Code review reminders, by giving a LGTM you attest that:

  • Commits are adequately tested
  • Code is easy to understand and conforms to style guides
  • Incomplete code is marked with TODOs
  • Code is suitably instrumented with logging and metrics

@maiamcc
Copy link
Contributor Author

maiamcc commented May 7, 2020

just running gsutil cp SHOULD overwrite the existing objects but I couldn't make that behavior happen so i went with this instead (--no_check doesn't guarantee overwriting, it just doesn't check). That might be unnecessary word mincing tho 🤷‍♀️

print("Error: Files already exist: %s" % url)
sys.exit(1)

if not args.no_check:
Copy link
Member

Choose a reason for hiding this comment

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

is there a risk this will get us into a weird state where two releases are on top of each other? do we need to delete the old folder if it exists?

@@ -13,7 +13,7 @@ fi
DIR=$(dirname "$0")
cd "$DIR/.."

./scripts/upload-assets.py latest
./scripts/upload-assets.py latest --force
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm... honestly not sure if we want this or if we want to allow users to pipe the --force arg thru via make release / have a separate invocation upload-assets.py latest --clean that JUST deletes the existing bucket and then user can run make release without the --force flag. thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I can come up with some pros and cons but they seem minor enough and the impact of this decision small enough that it's probably not worth our time.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I'm in favor of the --clean option. As Nick has been saying, we should move the release process to CI, and the --clean option allows us to handle that error separately on our laptops.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@maiamcc
Copy link
Contributor Author

maiamcc commented Sep 29, 2020

@nicks took another swing at this cuz it was bothering me this morning!

@maiamcc
Copy link
Contributor Author

maiamcc commented Oct 5, 2020

@nicks @landism ping! (This is pretty different from the original version so wants re-review)

@maiamcc maiamcc requested a review from nicks October 5, 2020 15:08
@landism
Copy link
Member

landism commented Oct 5, 2020

Alternate proposal, if you're interested:

change the asset server to always fetch assets by sha and take asset uploads out of the release process

@nicks
Copy link
Member

nicks commented Oct 5, 2020

alternative proposal #2 - bake assets into the binary, cf https://go.googlesource.com/proposal/+/master/design/draft-embed.md, #2608

@nicks
Copy link
Member

nicks commented Oct 5, 2020

actually, nm, i guess embedded assets wouldn't fix the whole problem, since we'd still have to do asset releases for snapshots

@maiamcc maiamcc merged commit 866fc3e into master Oct 6, 2020
@maiamcc maiamcc deleted the upload-assets-messaging branch October 6, 2020 21:05
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