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

Simplify deploy.sh to share most of the steps in most cases #137

Merged
merged 2 commits into from
Dec 20, 2017

Conversation

foolip
Copy link
Member

@foolip foolip commented Dec 15, 2017

The "Skipping deploy for a pull request" is removed, meaning that the
same steps will be run for both a "branch update" (a branch pushed to
the whatwg/foo repo) and a "pull request update" (a PR opened on the
whatwg/foo repo).

Just PR builds should be enough, and to avoid unnecessary work branch
builds could be limited to master in .travis.yml:
https://docs.travis-ci.com/user/customizing-the-build/#Safelisting-or-blocklisting-branches

There's no reason to check ENCRYPTION_LABEL up front, as the deploy
bits are guarded by being on Travis and master, where the secrets
should always be available.

The commit snapshots will be done on master whether on Travis or not,
so that it can be tested locally more easily.

@foolip
Copy link
Member Author

foolip commented Dec 15, 2017

Low priority, and depends on #135, will rebase when that's merged.

This change is in need of some discussion I think. We could just leave it all be, but I think it'd make sense to do the build and run the validator on all PRs, not just branches in the upstream WHATWG repos.

This will mean either running Travis twice for our own PRs, or that we edit .travis.yml everywhere. I was meaning to clean up DEPLOY_USER everywhere anyway, so can just bundle those changes if the work required is the only concern.

@foolip
Copy link
Member Author

foolip commented Dec 15, 2017

Rebased.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

So I think it's worth discussing what our cutoff is for what runs:

  • On master on CI
  • On PRs on CI
  • Locally on master
  • Locally on a branch

My initial suggestion:

  • On master on CI: everything
  • On PRs on CI: living standard build; HTML checker
  • Locally on master/locally on a branch: living standard build only. Maybe the service worker?

This PR does everything in all situations except skipping the commit-snapshot build for PRs and local branches. I could get behind doing everything in all situations, but that exception seems strange.

@foolip
Copy link
Member Author

foolip commented Dec 16, 2017

I'd be fine with building the commit snapshot unconditionally, once in a few years it might reveal a problem with that before it reaches master. I think encodings is the slowest thing right now because of the validator step, and that'd become twice as slow. @annevk, WDYT?

Trying to run the validator locally would also be good I think, but the /usr/lib/jvm/java-8-oracle/jre/bin/java bit gave me pause.

Other than taking "everything everywhere" as far as possible, I'd also be fine with "My initial suggestion" from #137 (review). That would suggest having an early exit for branch builds that aren't master, or doing it with .travis.yml everywhere.

@annevk
Copy link
Member

annevk commented Dec 16, 2017

I don't mind. Given all the other things going on with a PR before it can land the additional delay shouldn't be much of an issue. And if it is we can revisit.

@domenic
Copy link
Member

domenic commented Dec 19, 2017

I'm OK either way ("everything everywhere, minus the checker" or "my initial suggestion").

I do think CI for PRs being fast is nice, and that would be my default, but I don't think we're at a problematic point yet. And I guess local builds doing everything is a bit nicer since you can just use make or equivalent for the quick version.

@foolip
Copy link
Member Author

foolip commented Dec 19, 2017

Now reduced to "everything everywhere". I'm actually not very confident that we won't regret this, this makes any flakiness more likely to manifest. I tried to turn "my initial suggestion" into lines of code, but the missing part is what to do with branches in whatwg repos.

@domenic
Copy link
Member

domenic commented Dec 19, 2017

I think we can just not run Travis at all for branches in whatwg repos.

@foolip
Copy link
Member Author

foolip commented Dec 19, 2017

The trouble is, I think, that this would also exclude the master branch. I'd be happy to sprinkle https://docs.travis-ci.com/user/customizing-the-build/#Safelisting-or-blocklisting-branches everywhere as I remove DEPLOY_USER, though.

@domenic
Copy link
Member

domenic commented Dec 19, 2017

Right, that's what I was thinking.

foolip added a commit to whatwg/fetch that referenced this pull request Dec 19, 2017
The "Skipping deploy for a pull request" is removed, meaning that the
same steps will be run for both a "branch update" (a branch pushed to
the whatwg/foo repo) and a "pull request update" (a PR opened on the
whatwg/foo repo).

Just PR builds should be enough, and to avoid unnecessary work branch
builds could be limited to master in .travis.yml:
https://docs.travis-ci.com/user/customizing-the-build/#Safelisting-or-blocklisting-branches

There's no reason to check ENCRYPTION_LABEL up front, as the deploy
bits are guarded by being on Travis and master, where the secrets
should always be available.

The commit snapshots will be done on master whether on Travis or not,
so that it can be tested locally more easily.
@foolip
Copy link
Member Author

foolip commented Dec 19, 2017

OK, sent whatwg/fetch#648 to show what that would look like, and I could use that as the PR to test with. This PR is now updated a bit to more obviously just remove some if-guards and tweak the logging a bit.

The backup plan if this makes e.g. encoding to slow would be to put commit snapshot back behind some guard.

@annevk
Copy link
Member

annevk commented Dec 19, 2017

I think we can just not run Travis at all for branches in whatwg repos.

Wait why? I'd like to get feedback about Bikeshed warnings/errors and conformance checker output.

annevk pushed a commit to whatwg/fetch that referenced this pull request Dec 19, 2017
@domenic
Copy link
Member

domenic commented Dec 19, 2017

Wait why? I'd like to get feedback about Bikeshed warnings/errors and conformance checker output.

Clarified elsewhere, but the reason is that it's redundant with the PR Travis job that happens at the same time.

curl -O https://sideshowbarker.net/nightlies/jar/vnu.jar
/usr/lib/jvm/java-8-oracle/jre/bin/java -jar vnu.jar --skip-non-html --Werror --filterpattern "$CHECKER_FILTER" "$WEB_ROOT"
fi

# Deploy from Travis on master branch only
if [[ "$TRAVIS" == "true" && "$BRANCH" == "master" ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

I think this will attempt to deploy when building a pull request, as long as the PR is from the fork's master branch. (Because you deleted the guard against pull requests.)

Copy link
Member Author

Choose a reason for hiding this comment

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

After a bit of flailing about I confirmed that you're right in https://travis-ci.org/whatwg/encoding/builds/318824786, TRAVIS_BRANCH will be master even for a PR. (I thought it might be some generated name like pr/123/merged/with/master, but nope.)

Will fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I was still a bit wrong, the name of the source branch doesn't seem to matter, https://docs.travis-ci.com/user/environment-variables/#Default-Environment-Variables says "for builds triggered by a pull request this is the name of the branch targeted by the pull request".

@foolip
Copy link
Member Author

foolip commented Dec 19, 2017

OK, now the incorrect master deploy should be fixed, and I simplified away the BRANCH variable in the process.

annevk pushed a commit to whatwg/encoding that referenced this pull request Dec 20, 2017
@foolip foolip merged commit 8e28b23 into master Dec 20, 2017
@annevk annevk deleted the simplify-deploy branch December 20, 2017 08:43
foolip added a commit to whatwg/mimesniff that referenced this pull request Dec 20, 2017
foolip added a commit to whatwg/notifications that referenced this pull request Dec 20, 2017
foolip added a commit to whatwg/quirks that referenced this pull request Dec 20, 2017
annevk pushed a commit to whatwg/dom that referenced this pull request Dec 20, 2017
annevk pushed a commit to whatwg/fullscreen that referenced this pull request Dec 20, 2017
annevk pushed a commit to whatwg/infra that referenced this pull request Dec 20, 2017
foolip added a commit to whatwg/storage that referenced this pull request Dec 20, 2017
foolip added a commit to whatwg/streams that referenced this pull request Dec 20, 2017
foolip added a commit to whatwg/url that referenced this pull request Dec 20, 2017
foolip added a commit to whatwg/xhr that referenced this pull request Dec 20, 2017
annevk pushed a commit to whatwg/mimesniff that referenced this pull request Dec 20, 2017
annevk pushed a commit to whatwg/notifications that referenced this pull request Dec 20, 2017
annevk pushed a commit to whatwg/url that referenced this pull request Dec 20, 2017
annevk pushed a commit to whatwg/quirks that referenced this pull request Dec 20, 2017
annevk pushed a commit to whatwg/storage that referenced this pull request Dec 20, 2017
annevk pushed a commit to whatwg/xhr that referenced this pull request Dec 20, 2017
domenic pushed a commit to whatwg/streams that referenced this pull request Dec 20, 2017
miketaylr pushed a commit to whatwg/compat that referenced this pull request Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants