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

Notify people of export PRs that bypass review #65

Open
foolip opened this issue Feb 21, 2019 · 6 comments
Open

Notify people of export PRs that bypass review #65

foolip opened this issue Feb 21, 2019 · 6 comments

Comments

@foolip
Copy link
Member

foolip commented Feb 21, 2019

In web-platform-tests/wpt#15306 @marcoscaceres asked about being notified of changes to payment-request/. Currently, @wpt-pr-bot will approve export PRs:

// Auto-approve PR that have been reviewed downstream.
if (metadata.reviewedDownstream) {
return approve(number, 'Already reviewed downstream.');
}

Review on the PR itself wouldn't be meaningful, and sometimes PRs are created for changes that never land even in the upstream repository, so if people are notified doing so after the PR has been merged is probably best.

Notifying in a comment would be OK because no other humans are looking at these PRs. But it has to be simple to filter out these notifications.

@foolip
Copy link
Member Author

foolip commented Feb 21, 2019

@annevk I think you've asked about this as well, do you remember where the previous discussion happened?

@marcoscaceres
Copy link

This can really help with planning - as it allows us (i.e., Mozilla's DOM Team) to start thinking about how to fix stuff and get things prioritized when updated tests fail. It also helps catch little things, like things that are .tentative. (but not marked as such) slipping through. Even if those things get auto-merged, we are potentially able to respond more quickly.

@gsnedders
Copy link
Member

@foolip the discussion previously might just have been on IRC; I certainly would like to know everything touching infra at least.

@annevk
Copy link
Member

annevk commented Feb 21, 2019

@foolip web-platform-tests/wpt#8434 is somewhat related.

(If the distinction could be made, I'd be interested in seeing changes to existing tests, but not so much in the addition of new tests or tentative tests. That's where I've mainly seen errors introduced at least, which is what I care about the most.)

@foolip
Copy link
Member Author

foolip commented Feb 21, 2019

If the distinction could be made, I'd be interested in seeing changes to existing tests, but not so much in the addition of new tests or tentative tests.

This is technically straightforward to detect, but to notify or not depending on it would require per-user settings. I guess those could live in the META.yml files, is that what you'd like?

@annevk
Copy link
Member

annevk commented Feb 21, 2019

Sounds great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants