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

Project Management Automation: Support adding milestones for fork PRs. #20058

Merged

Conversation

@epiqueras
Copy link
Contributor

epiqueras commented Feb 5, 2020

Follows #20049.

Separately, there might be alternatives to explore here, where the event is handled in response to the commit as it ends up being pushed to master. In other words, some combination of:

  • The push event
  • Conditioning on commits that occur in the master branch
  • Deriving a pull request associated with the commit
  • Applying milestone to the pull request
.github/workflows/pull-request-automation.yml Outdated Show resolved Hide resolved
@@ -25,8 +25,7 @@ const automations = [
task: ifNotFork( addFirstTimeContributorLabel ),
},
{
event: 'pull_request',
action: 'closed',
event: 'push',
task: ifNotFork( addMilestone ),

This comment has been minimized.

Copy link
@aduth

aduth Feb 6, 2020

Member

Is ifNotFork necessary anymore?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Feb 6, 2020

Author Contributor

Yes, why wouldn't it be?

This comment has been minimized.

Copy link
@aduth

aduth Feb 6, 2020

Member

It doesn't seem we would ever have a scenario where we would receive a push event and also not immediately bail at the first condition checking the master branch (i.e. I can't see how an error would ever occur if the ifNotFork was not here). I suppose it might still be better to stop it as early as possible to avoid tying it to this specific implementation.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Feb 6, 2020

Author Contributor

Forks can also have branches named master.

This comment has been minimized.

Copy link
@aduth

aduth Feb 6, 2020

Member

Are we receiving push events for those?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Feb 6, 2020

Author Contributor

Yes, someone could make a PR from fork/master.

This comment has been minimized.

Copy link
@aduth

aduth Feb 6, 2020

Member

Hm, ok. I think I may have been expecting that this condition would somehow already be verifying that the commit happened on wordpress/gutenberg's master, but I see now that's not the case.

But ifNotFork was implemented for pull request events, and I think would need to be updated to support this.

payload.pull_request.head.repo.full_name ===
payload.pull_request.base.repo.full_name

Notably, payload.pull_request doesn't exist for push events.

https://developer.github.com/v3/activity/events/types/#pushevent

This comment has been minimized.

Copy link
@epiqueras

epiqueras Feb 6, 2020

Author Contributor

Then it's better to just not use it. I've removed it. No one actually makes PRs from master anyway. We can live without milestones for those edge cases.

This comment has been minimized.

Copy link
@aduth

aduth Feb 6, 2020

Member

Then it's better to just not use it. I've removed it. No one actually makes PRs from master anyway. We can live without milestones for those edge cases.

I don't know how to search for specific examples to share, but I've definitely seen pulls from forked master before. That said, the combination of this and a commit message which matches the (#PR) pattern to get to the point of adding the milestone would be exceedingly rare, yes. And the result would merely be a failing action, nothing too worrisome.

We might want to revisit this in the future to make the logic more durable and to support ifNotFork usage for non-pull_request events (especially given the fact the name is generic enough to easily misinterpret that it ought be supported), but I don't think it's a blocker.

epiqueras and others added 2 commits Feb 6, 2020
Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>
@aduth
aduth approved these changes Feb 6, 2020
Copy link
Member

aduth left a comment

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Feb 6, 2020

@aduth
aduth approved these changes Feb 6, 2020
@epiqueras epiqueras merged commit 164d583 into master Feb 6, 2020
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@epiqueras epiqueras deleted the add/project-management-automation-milestones-for-fork-prs branch Feb 6, 2020
chipsnyder added a commit that referenced this pull request Feb 7, 2020
#20058)

* Project Management Automation: Support adding milestones for fork PRs.

* Github: Remove unused workflow event.

* Update packages/project-management-automation/lib/add-milestone.js

Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>

* Project Management Automation: Update tests.

* Project Management Automation: Remove `ifNotFork` from milestone handler.

Co-authored-by: Andrew Duthie <andrew@andrewduthie.com>
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 10, 2020

Small issue with the implementation which causes errors for commits to master: #20144

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Feb 10, 2020

Good catch! Fixed: #20147.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Tightening Up
  
Awaiting triage
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.