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

Jump to bottom

Fix wpt rev-list failure-handling in epochs_update.sh #24560

Merged
merged 1 commit into from Jul 16, 2020
Merged

Fix wpt rev-list failure-handling in epochs_update.sh #24560

merged 1 commit into from Jul 16, 2020

Conversation

stephenmcgruer
Copy link
Contributor

Previously if this subcommand failed it would be converted to the empty
string, which would then branch and push epochs at HEAD! This PR both
fixes the code to fail if the wpt subcommand fails, and also to
double-check that the SHA isn't empty.

Fixes #24504

@stephenmcgruer
Fix wpt rev-list failure-handling in epochs_update.sh
a14ef91 
Previously if this subcommand failed it would be converted to the empty
string, which would then branch and push epochs at HEAD! This PR both
fixes the code to fail if the wpt subcommand fails, and also to
double-check that the SHA isn't empty.

Fixes #24504
@wpt-pr-bot wpt-pr-bot requested a deployment to wpt-preview-24560 July 10, 2020 14:46 Pending
Hexcles
Hexcles reviewed Jul 10, 2020
View reviewed changes
Copy link
Member

@Hexcles Hexcles left a comment

Choose a reason for hiding this comment

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

Should wpt rev-list simply return a non-zero exit code when the epoch SHA can't be found, in which case this script would also abort due to set -e?

@jgraham
Copy link
Contributor

jgraham commented Jul 10, 2020

In the longer term it would be good to get rid of this kind of shell script; I think basically every time we replaced shell scripts with Python it ended up being easier to maintain.

@stephenmcgruer
Copy link
Contributor Author

Should wpt rev-list simply return a non-zero exit code when the epoch SHA can't be found, in which case this script would also abort due to set -e?

I'm happy to do that, but note the defense in depth here was supposed to guard against wpt rev-list being buggy. Plus I was trying to keep the fix focused. But lmk which way you'd like to go, still happy to do it.

In the longer term it would be good to get rid of this kind of shell script; I think basically every time we replaced shell scripts with Python it ended up being easier to maintain.

Yeah, I presume originally the idea was that the script wrapped the easy stuff and ./wpt rev-list --epoch ${EPOCH} was doing the hard bit. As it turns out, the script does a bit more than that.

Hexcles
Hexcles approved these changes Jul 10, 2020
View reviewed changes
Copy link
Member

@Hexcles Hexcles left a comment

Choose a reason for hiding this comment

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

I'm happy to do that, but note the defense in depth here was supposed to guard against wpt rev-list being buggy. Plus I was trying to keep the fix focused. But lmk which way you'd like to go, still happy to do it.

Fair enough. LGTM!

@stephenmcgruer stephenmcgruer merged commit b448d54 into master Jul 16, 2020
@stephenmcgruer stephenmcgruer deleted the smcgruer/fix-epochs branch July 16, 2020 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hexcles Hexcles Hexcles approved these changes

@jgraham jgraham Awaiting requested review from jgraham

@LukeZielinski LukeZielinski Awaiting requested review from LukeZielinski

Assignees

@Hexcles Hexcles

Labels
ci infra
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

epochs_update.sh badly handles ./wpt rev-list --epoch ${EPOCH} failing
4 participants
@stephenmcgruer @jgraham @Hexcles @wpt-pr-bot