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

Unreviewed commits on master #19311

Closed
foolip opened this issue Sep 25, 2019 · 8 comments
Closed

Unreviewed commits on master #19311

foolip opened this issue Sep 25, 2019 · 8 comments
Assignees
@gsnedders

Comments

@foolip
Copy link
Member

foolip commented Sep 25, 2019

These 3 commits from May 14 seem to have landed without going through review:
39e5c2b
e0fd5d5
68ceaae

I noticed because for the first and last there are wpt.fyi runs:
https://wpt.fyi/runs?sha=39e5c2b4a6da7d2a619364b556c5ec59a934c89b
https://wpt.fyi/runs?sha=68ceaae7aecf1afbc7f0698ee6ba8c6c7097363d

The total diff introduced by these commits:

--- a/tools/ci/website_build.sh
+++ b/tools/ci/website_build.sh
@@ -56,12 +56,12 @@ git config --global user.name "wpt-pr-bot"
 
 # Prepare the output directory so that the new build can be pushed to the
 # repository as an incremental change to the prior build.
-mkdir --parents docs/_build/html
+mkdir -p docs/_build/html
 cd docs/_build/html
 git init
 git fetch --depth 1 ${remote_url} gh-pages
 git checkout FETCH_HEAD
-git rm -r .
+git rm -rf .
 
 # Build the website
 cd ../..
@@ -78,7 +78,7 @@ touch .nojekyll
 # Publish the website by pushing the built contents to the `gh-pages` branch
 git add .
 
-if ! git diff --exit-code --quiet --staged ; then
+if git diff --exit-code --quiet --staged ; then
   echo No change to the website contents. Exiting without publishing.
 
   exit ${neutral_status}

@gsnedders @jugglinmike do either of you know what happened here? Doesn't branch protection prevent this from happening?
@foolip
Copy link
Member Author

foolip commented Sep 25, 2019

5612fb4 is in this same category (wpt.fyi runs labeled master that don't have merge_pr_* tags) and explained by #16806. So it looks like @gsnedders was pushing to master in order to fix the docs build.

Leaving open to confirm.

@foolip foolip changed the title 3 unreviewed commits on master Unreviewed commits on master Sep 25, 2019
@foolip
Copy link
Member Author

foolip commented Sep 25, 2019

Just a few more to check, but is 78384fb is another. @sideshowbarker I've looked through PRs but can't find anything, and https://api.github.com/search/issues?q=type:pr+is:merged+repo:web-platform-tests/wpt+sha:78384fb41b6f678b2d9a3ebc2bf9f0fdbc1277c2 doesn't return any either.

Do you think you accidentally pushed this directly to master?

@jugglinmike
Copy link
Contributor

it looks like @gsnedders was pushing to master in order to fix the docs build.

That's correct. Sam and I were working closely together on this. We would of course prefer not to treat master as a testing ground, but it was unavoidable in this case.

We'd previously vetted the patch and its integration with GitHub using the master branch of an independent repository/GitHub project. The issues we were ironing out were related to details that were only apparent when using the canonical repository.

@foolip
Copy link
Member Author

foolip commented Sep 25, 2019

The final commit on my list to investigate was c717f05, which I explained in #18717 (comment) I accidentally pushed directly to master.

If @sideshowbarker's 78384fb was also a mistake, then perhaps we should enforce branch protection for admins as well.

Nothing very bad has happened as a consequence of this so far, but it does mean that there are some runs in wpt.fyi which were from master for which we can't get a copy of the manifest from releases in this repo. @Hexcles FYI.

@foolip foolip mentioned this issue Sep 25, 2019
Open
@sideshowbarker
Copy link
Contributor

If @sideshowbarker's 78384fb was also a mistake

Looking at 78384fb, I see the commit was associated with hoojaoh#1565, which is a push from the "pull" bot that got merged in the hoojaoh:master branch of the hoojaoh/web-platform-tests for. At this point, I don’t actually have any idea what happened there. I guess the bot pulled some change that had already been committed to master?

Or maybe that’s all irrelevant, since it was change to a branch on fork. I guess maybe GitHub associated it with that PR because it was the only PR it could find to associate it with?

Anyway, regardless, at this point, I don’t actually recall anything about 78384fb.

then perhaps we should enforce branch protection for admins as well

I think we should enforce branch protection for admins, regardless. I thought we were already doing that, actually. I don’t see any reason why we shouldn’t.

But would enforcing branch protection for admins also have prevented the push from that bot?

@foolip
Copy link
Member Author

foolip commented Oct 8, 2019

In https://github.com/web-platform-tests/wpt/settings/branch_protection_rules/1493 I just tried checking this checkbox:
image

However, as a result it's not longer possible to admin merge a PR, that is the following UI no longer appears on a PR like #19377:
image

So I've unchecked it again. It would be great to get to a state where we never need to override a failing Taskcluster status, and perhaps with @KyleJu's work to annotate known flakiness we'll get close, but for now we still need to admin merge.

There isn't anything actionable in this issue and I've already asked about or accounted for the unreviewed commits, so closing this.

@web-platform-tests/admins FYI.

@foolip foolip closed this as completed Oct 8, 2019
@foolip
Copy link
Member Author

foolip commented Oct 18, 2019

Oops, I just produced two more commits. I was using the VS Code GitHub integration and had set my upstream to 'master' when I tried to push the branch and create a pull request. I reverted immediately but it resulting in these two commits: 9e1d0a2, f3c657a

Sorry!

foolip referenced this issue Oct 18, 2019
@foolip
[Azure Pipelines] drop --exclude workarounds
9e1d0a2
foolip referenced this issue Oct 18, 2019
@foolip
Revert "[Azure Pipelines] drop --exclude workarounds"
f3c657a 
This reverts commit 9e1d0a2.
This was referenced Oct 18, 2019
Closed
Closed
Merged
@foolip
Copy link
Member Author

foolip commented Oct 26, 2019

Hah, I did it again! This time I blame the GitHub web UI:
4c854cc#commitcomment-35674629

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gsnedders gsnedders

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@gsnedders @sideshowbarker @foolip @jugglinmike