-
Notifications
You must be signed in to change notification settings - Fork 50
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
WIP review draft deploy #201
Conversation
resources.whatwg.org/build/deploy.sh
Outdated
for f in "$REVIEW_DRAFTS_DIR"/*.bs; do | ||
[ -e "$f" ] || continue # http://mywiki.wooledge.org/BashPitfalls#line-80 | ||
if [[ "$TRAVIS_PULL_REQUEST" == "true" ]]; then | ||
CHANGED_FILES=$(git diff --name-only master..HEAD) |
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.
Since we put the DATE directly in the draft I don't think we need to distinguish between added and changed anymore. Hence we can simply use --name-only instead of --name-status.
I used master..HEAD here so a PR will consider all commits. We'll just have to ensure to land as a single commit on master.
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.
Nice!
resources.whatwg.org/build/deploy.sh
Outdated
CHANGED_FILES=$(git diff --name-only HEAD~1) | ||
fi | ||
for change in "$CHANGED_FILES"; do | ||
if [[ f != change ]]; 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.
Or should these variables be quoted again here? It's also a little unclear to me when to user lowercase and when to use uppercase for variables.
resources.whatwg.org/build/deploy.sh
Outdated
BASENAME=$(basename "$f" .bs) | ||
DRAFT_DIR="$WEB_ROOT/$REVIEW_DRAFTS_DIR/$BASENAME" | ||
mkdir -p "$DRAFT_DIR" | ||
curlbikeshed -F md-Status="RD" \ |
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.
Should this be whatwg/RD?
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.
Nah, since we're already in the WHATWG org, just the status suffices.
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.
Will play around with this later, great stuff.
resources.whatwg.org/build/deploy.sh
Outdated
for f in "$REVIEW_DRAFTS_DIR"/*.bs; do | ||
[ -e "$f" ] || continue # http://mywiki.wooledge.org/BashPitfalls#line-80 | ||
if [[ "$TRAVIS_PULL_REQUEST" == "true" ]]; then | ||
CHANGED_FILES=$(git diff --name-only master..HEAD) |
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.
Nice!
resources.whatwg.org/build/deploy.sh
Outdated
BASENAME=$(basename "$f" .bs) | ||
DRAFT_DIR="$WEB_ROOT/$REVIEW_DRAFTS_DIR/$BASENAME" | ||
mkdir -p "$DRAFT_DIR" | ||
curlbikeshed -F md-Status="RD" \ |
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.
Nah, since we're already in the WHATWG org, just the status suffices.
resources.whatwg.org/build/deploy.sh
Outdated
else | ||
CHANGED_FILES=$(git diff --name-only HEAD~1) | ||
fi | ||
for change in $CHANGED_FILES; do # Omit quotes around variable to split on whitespace |
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.
FWIW we should probably stick to uppercase everywhere, I was just copying and pasting from places that did lowercase.
resources.whatwg.org/build/review.sh
Outdated
INPUT_FILE=$(find . -maxdepth 1 -name "*.bs" -print -quit) | ||
REVIEW_DRAFT="review-drafts/$(date +'%Y-%m').bs" | ||
# The very unusual way of creating a newline here is done to keep macOS happy | ||
sed 's/^Group: WHATWG$/&'''$'\n'"Date: $(date +'%Y-%m-%d')/g" < "$INPUT_FILE" > "$REVIEW_DRAFT" |
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 tried running this locally and got sed: -e expression #1, char 19: unterminated `s' command
which is probably about this line. Using sed (GNU sed) 4.4.
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.
Hmm, I suspect the various versions of sed are incompatible in their syntaxes. What matters most is that this works on Travis, which we haven't really tried yet. But it does pass shellcheck apparently.
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 '''
also does not work on macOS. \'
did work.
While working on whatwg/html-build#151 I found a number of issues with the approach here. Will try to fix. |
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.
This should all be in order given that equivalent scripts worked for HTML and most of this was tested locally too.
See #197 and whatwg/sg#70.
This doesn't yet prevent re-building and deploying; discussions ongoing in #197.