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.

Sign up for GitHub

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
merged 5 commits into from
Feb 6, 2020
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3a80823
Project Management Automation: Support adding milestones for fork PRs.
epiqueras Feb 5, 2020
81a7d17
Github: Remove unused workflow event.
epiqueras Feb 6, 2020
ad855f5
Update packages/project-management-automation/lib/add-milestone.js
epiqueras Feb 6, 2020
4b005c6
Project Management Automation: Update tests.
epiqueras Feb 6, 2020
06d4677
Project Management Automation: Remove `ifNotFork` from milestone hand…
epiqueras Feb 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/pull-request-automation.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
on:
pull_request:
types: [opened, closed]
types: [opened]
push:
name: Pull request automation

jobs:
Expand Down
19 changes: 9 additions & 10 deletions packages/project-management-automation/lib/add-milestone.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,18 @@ const isDuplicateValidationError = ( error ) =>
/**
* Assigns the correct milestone to PRs once merged.
*
* @param {Object} payload Pull request event payload, see https://developer.github.com/v3/activity/events/types/#pullrequestevent.
* @param {Object} payload Push event payload, see https://developer.github.com/v3/activity/events/types/#pushevent.
* @param {Object} octokit Initialized Octokit REST client, see https://octokit.github.io/rest.js/.
*/
async function addMilestone( payload, octokit ) {
if ( ! payload.pull_request.merged ) {
debug( 'add-milestone: Pull request is not merged. Aborting' );
if ( payload.ref !== 'refs/heads/master' ) {
debug( 'add-milestone: Commit is not to `master`. Aborting' );
return;
}

if ( payload.pull_request.base.ref !== 'master' ) {
debug(
'add-milestone: Pull request is not based on `master`. Aborting'
);
const [ , prNumber ] = payload.commits[ 0 ].message.match( /\(#(\d+)\)$/m );
if ( ! prNumber ) {
debug( 'add-milestone: Commit is not a squashed PR. Aborting' );
return;
}

Expand All @@ -55,7 +54,7 @@ async function addMilestone( payload, octokit ) {
} = await octokit.issues.get( {
owner: payload.repository.owner.login,
repo: payload.repository.name,
issue_number: payload.pull_request.number,
issue_number: prNumber,
} );

if ( milestone ) {
Expand Down Expand Up @@ -133,13 +132,13 @@ async function addMilestone( payload, octokit ) {
);

debug(
`add-milestone: Adding issue #${ payload.pull_request.number } to milestone #${ number }`
`add-milestone: Adding issue #${ prNumber } to milestone #${ number }`
);

await octokit.issues.update( {
owner: payload.repository.owner.login,
repo: payload.repository.name,
issue_number: payload.pull_request.number,
issue_number: prNumber,
milestone: number,
} );
}
Expand Down
5 changes: 2 additions & 3 deletions packages/project-management-automation/lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@ const automations = [
task: ifNotFork( addFirstTimeContributorLabel ),
},
{
event: 'pull_request',
action: 'closed',
event: 'push',
task: ifNotFork( addMilestone ),
Copy link
Member

Choose a reason for hiding this comment

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

Is ifNotFork necessary anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, why wouldn't it be?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forks can also have branches named master.

Copy link
Member

Choose a reason for hiding this comment

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

Are we receiving push events for those?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@aduth aduth Feb 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

gutenberg/packages/project-management-automation/lib/if-not-fork.js

Lines 18 to 19 in cf1a6d0

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

},
];
Expand All @@ -47,7 +46,7 @@ const automations = [
for ( const { event, action, task } of automations ) {
if (
event === context.eventName &&
action === context.payload.action
( action === undefined || action === context.payload.action )
) {
try {
debug( `main: Starting task ${ task.name }` );
Expand Down
73 changes: 13 additions & 60 deletions packages/project-management-automation/lib/test/add-milestone.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,42 +3,10 @@
*/
import addMilestone from '../add-milestone';

describe( 'addFirstTimeContributorLabel', () => {
it( 'does nothing if PR is not merged', async () => {
const payload = {
pull_request: {
merged: false,
},
};
const octokit = {
issues: {
get: jest.fn(),
createMilestone: jest.fn(),
listMilestonesForRepo: jest.fn(),
update: jest.fn(),
},
repos: {
getContents: jest.fn(),
},
};

await addMilestone( payload, octokit );

expect( octokit.issues.get ).not.toHaveBeenCalled();
expect( octokit.issues.createMilestone ).not.toHaveBeenCalled();
expect( octokit.issues.listMilestonesForRepo ).not.toHaveBeenCalled();
expect( octokit.issues.update ).not.toHaveBeenCalled();
expect( octokit.repos.getContents ).not.toHaveBeenCalled();
} );

describe( 'addMilestone', () => {
it( 'does nothing if base is not master', async () => {
const payload = {
pull_request: {
merged: true,
base: {
ref: 'release/5.0',
},
},
ref: 'refs/heads/not-master',
};
const octokit = {
issues: {
Expand All @@ -63,13 +31,8 @@ describe( 'addFirstTimeContributorLabel', () => {

it( 'does nothing if PR already has a milestone', async () => {
const payload = {
pull_request: {
merged: true,
base: {
ref: 'master',
},
number: 123,
},
ref: 'refs/heads/master',
commits: [ { message: '(#123)' } ],
repository: {
owner: {
login: 'WordPress',
Expand Down Expand Up @@ -100,7 +63,7 @@ describe( 'addFirstTimeContributorLabel', () => {
expect( octokit.issues.get ).toHaveBeenCalledWith( {
owner: 'WordPress',
repo: 'gutenberg',
issue_number: 123,
issue_number: '123',
} );
expect( octokit.issues.createMilestone ).not.toHaveBeenCalled();
expect( octokit.issues.listMilestonesForRepo ).not.toHaveBeenCalled();
Expand All @@ -110,13 +73,8 @@ describe( 'addFirstTimeContributorLabel', () => {

it( 'correctly milestones PRs when `package.json` has a `*.[1-8]` version', async () => {
const payload = {
pull_request: {
merged: true,
base: {
ref: 'master',
},
number: 123,
},
ref: 'refs/heads/master',
commits: [ { message: '(#123)' } ],
repository: {
owner: {
login: 'WordPress',
Expand Down Expand Up @@ -166,7 +124,7 @@ describe( 'addFirstTimeContributorLabel', () => {
expect( octokit.issues.get ).toHaveBeenCalledWith( {
owner: 'WordPress',
repo: 'gutenberg',
issue_number: 123,
issue_number: '123',
} );
expect( octokit.repos.getContents ).toHaveBeenCalledWith( {
owner: 'WordPress',
Expand All @@ -186,20 +144,15 @@ describe( 'addFirstTimeContributorLabel', () => {
expect( octokit.issues.update ).toHaveBeenCalledWith( {
owner: 'WordPress',
repo: 'gutenberg',
issue_number: 123,
issue_number: '123',
milestone: 12,
} );
} );

it( 'correctly milestones PRs when `package.json` has a `*.9` version', async () => {
const payload = {
pull_request: {
merged: true,
base: {
ref: 'master',
},
number: 123,
},
ref: 'refs/heads/master',
commits: [ { message: '(#123)' } ],
repository: {
owner: {
login: 'WordPress',
Expand Down Expand Up @@ -249,7 +202,7 @@ describe( 'addFirstTimeContributorLabel', () => {
expect( octokit.issues.get ).toHaveBeenCalledWith( {
owner: 'WordPress',
repo: 'gutenberg',
issue_number: 123,
issue_number: '123',
} );
expect( octokit.repos.getContents ).toHaveBeenCalledWith( {
owner: 'WordPress',
Expand All @@ -269,7 +222,7 @@ describe( 'addFirstTimeContributorLabel', () => {
expect( octokit.issues.update ).toHaveBeenCalledWith( {
owner: 'WordPress',
repo: 'gutenberg',
issue_number: 123,
issue_number: '123',
milestone: 12,
} );
} );
Expand Down