WordPress.org

Making WordPress.org

Opened 4 months ago

Last modified 5 days ago

#4903 new enhancement

Show Github PRs on Trac

Reported by: dd32 Owned by:
Milestone: Priority: normal
Component: Trac Keywords:
Cc:

Description (last modified by dd32)

With Github PRs being used more often now, there's been an attempt by a few people to expose Github PRs within Trac, this ticket is being used for tracking that effort.

Initially this is going to be enabled for Core trac only, and for the WordPress/WordPress-develop Github repo only, but has the potential to have multiple repo's and multiple tracs in the future if needed.

Initial development of this was worked on by @andraganescu @desrosj @isabel_brison @noisysocks @pento @talldanwp

The commits that follow this have however been rewritten heavily by myself to make it performant and fit the WordPress.org style.


Task List: ( ✅Completed, ❌Not yet, ❓Pending Question)
✅Track PRs on Github, and show them on Trac Tickets
✅Add a mention to Tickets comments when a PR is opened.
✅Always load PRs, for authenticated and unauthenticated (Currently it only auto-loads for authenticated requests)
✅Sync PR Comments to Trac
✅ Switch to using a Github App OAuth Token from dd32's personal Github token get_authorization_token() (or change it to select a random users token..)
❌Sync PR Comment edits to Trac
❌Sync PR Code Review comments to Trac #5053
❌Sync PR Code Review comment edits to Trac
❓Should all Github PR comments trigger Trac Emails? (Note: Edits won't be able to, and responses to existing code review instances would be best done as edits too)
❓Should Github PR activity be piped to Trac Firehose channels to compensate for lack of edit emails?

Change History (40)

#1 @dd32
4 months ago

In 9340:

Trac: Add initial code to show Github PRs on Trac tickets.

Props dd32, andraganescu, desrosj, isabel_brison, noisysocks, pento, talldanwp.
See #4903.

#2 @dd32
4 months ago

In 9341:

Trac: Bump scripts version.

See #4903.

#3 @dd32
4 months ago

The Core Trac caches are slowly clearing.. #core43805 is an example of a ticket with a single PR, and #core48502 for 3 PRs.

#4 @Mte90
4 months ago

Hi sorry for the questions, but I have few of them because this is implementation is revolutionary:

  • When the handbook will be updated? In this way we can suggest an alternative and more easy way instead of doing patches also at contributors day
  • This pr will be tracked inside Trac in the My Patches? I think that this require https://meta.trac.wordpress.org/ticket/4447 enabled for everyone
  • I think that also few guidelines about how to do the pr in the handbook will be very helpful, I think that for the next patch I will do I will switch to GitHub.

#5 @dd32
4 months ago

Hi @Mte90 and thanks for asking

When the handbook will be updated? In this way we can suggest an alternative and more easy way instead of doing patches also at contributors day

It's up to the Core team as to when that happens, there's a few more things in the works that a few believe is needed before full recommendation of it.

This pr will be tracked inside Trac in the My Patches?

At this point, there's no plans for that. Unfortunately integrating with https://core.trac.wordpress.org/my-patches would require some significant changes to how the Trac query happens.
There's the option that we could potentially just include an extra list below the table of any open PRs, but that's not high on my list of priorities for now.

There's a more-or-less hidden (just unannounced) feature in profiles - you can link your Github account with your WordPress.org account which helps #4447 greatly.

I think that also few guidelines about how to do the pr in the handbook will be very helpful

Once again, that's up to the Core team to update their handbooks - AFAIK there's no real current guidelines, but I expect that would happen pretty quickly if PR quality becomes an issue.

#6 @dd32
4 months ago

In 9365:

Trac: Github PRs: Properly craft the Github API request when a POST request is used, and ensure that we set the headers correctly.

See #4903.

#7 @dd32
4 months ago

In 9366:

Trac: Github PRs: When a PR is first detected referencing a ticket, add an inline mention to Trac.

For now, this doesn't trigger email notifications.

See #4903.

#8 @dd32
4 months ago

In 9367:

Trac: Github PRs: style prbot comments similar to slackbot mentions when the comment is italic (which signifies it's an inline comment, rather than a regular comment).

See #4903.

#9 @dd32
4 months ago

See also [9356] which still hasn't actually been deployed

Trac: Increase the padding of the 'chat-bot' comments (slackbot, etc) by 1px to hide the underline from the parent element.
This also hides the comment action buttons since they're not useful here.

#10 @dd32
4 months ago

In 9368:

Trac: Bump scripts version again to deploy r9356.

See #4903.

#11 @dd32
4 months ago

In 9370:

Trac: Github PRs: Add a wrapper to call the trac wpapi endpoints.

See #4903.

#12 @dd32
4 months ago

In 9371:

Trac: Github PRs: Fix a typo preventing finding the WordPress.org user.

See #4903.

#13 @dd32
4 months ago

In 9372:

Trac: Github PRs: Add a wrapper for the Trac class for easier testing.

See #4903.

#14 @dd32
4 months ago

  • Description modified (diff)
  • Type changed from defect to enhancement

Adding a basic task list to the ticket description to track current and future parts of this ticket that's needed.

This ticket was mentioned in Slack in #core by sergey. View the logs.


3 months ago

#16 @dd32
7 weeks ago

In 9510:

Trac: Expand the Github PR section by default for all users, not just authenticated ones.

See #4903.

#17 @dd32
7 weeks ago

In 9511:

Trac: Update the 'no PRs found' text for the Github PRs on Trac.

Props noisysocks.
See #4903.
Fixes #5042.

#18 @dd32
7 weeks ago

In 9513:

Trac: Bump scripts version.

See #4903, #5042.

#19 @dd32
7 weeks ago

In 9514:

Trac: Remove the no-longer-used PRNumber trac field.

Props noisysocks.
See #4903.

#20 @dd32
7 weeks ago

  • Description modified (diff)

This ticket was mentioned in Slack in #core by sergey. View the logs.


7 weeks ago

This ticket was mentioned in Slack in #core by afercia. View the logs.


7 weeks ago

#23 @desrosj
7 weeks ago

Some related changes elsewhere:

#24 @dd32
6 weeks ago

  • Description modified (diff)

See also: #5052, #5052, #5054

#25 follow-up: @ocean90
6 weeks ago

I think the display of mergeable_state needs some improvement. When a build is still running the value is unstable which is incorrectly displayed as "❌ Failing tests". When tests are actually failing (example) the state is unknown and displayed as "undefined".

It seems like https://developer.github.com/v3/repos/statuses/#get-the-combined-status-for-a-specific-ref will give us the correct results for tests.

Last edited 6 weeks ago by ocean90 (previous) (diff)

#26 @dd32
6 weeks ago

In 9534:

Trac: Github PRs: Use the Github .diff rather than .patch as they can differ in subtle ways.

Props azaozz, noisysocks.
See #4903.

#27 in reply to: ↑ 25 @dd32
6 weeks ago

Replying to ocean90:

I think the display of mergeable_state needs some improvement. When a build is still running the value is unstable which is incorrectly displayed as "❌ Failing tests". When tests are actually failing (example) the state is unknown and displayed as "undefined".

Agreed, The logic is in the prStatus() function in the JS, if someone could properly list out the actual statuses that we need to take into account. I just copied what the previous implementation had used, which evidently doesn't cover all bases.

It seems like https://developer.github.com/v3/repos/statuses/#get-the-combined-status-for-a-specific-ref will give us the correct results for tests.

PR's don't link to that endpoint directly, but crafting the URL for it seems to work.
The only "issue" I see is that the output only ever mentions AppVeyor, not Travis.. I'm unsure if that's just because it's the slower of the two, but the Github docs seem to suggest it should include both? (It's github.. it probably works)
eg. pulls/166 links to /statuses/{SHA} which can be altered to a {SHA}/status request.

#28 @dd32
5 weeks ago

In 9564:

Trac: Github PRs: Switch to using a Github App Token for authentication with Github.

This significantly increases the number of API requests that can be made per hour, and enables access to several additional API endpoints.

This uses the MIT licensed https://github.com/adhocore/php-jwt to generate the JWTs as required by the Github App API.

See #4903, #5052.

#29 @dd32
5 weeks ago

  • Description modified (diff)

#30 follow-up: @garrett-eclipse
5 weeks ago

@dd32 using the feature it would be nice if grunt patch:XXXX would list not only uploaded .diff patches but also PRs so from the list a user could choose to patch using either.
(I can open a new ticket for this)

This would simplify the patching process as you wouldn't need to find the PR URL and apply it as follows;
grunt patch:http://github.../wordpress-develop/pull/<id>

As well it would avoid situations where there's some svn diffs but a newer PR and users used to running grunt patch don't overlook the fact there's a PR that's actually newer than any of the diffs that'd be listed by the grunt command.

P.S. Awesome work here, thanks

#31 in reply to: ↑ 30 @dd32
5 weeks ago

Replying to garrett-eclipse:

@dd32 using the feature it would be nice if grunt patch:XXXX would list not only uploaded .diff patches but also PRs so from the list a user could choose to patch using either.

You can already pass a PR url to grunt:patch and it'll work apparently.

(I can open a new ticket for this)

The best place would be to file an issue on WordPress/grunt-patch-wordpress.

This ticket was mentioned in PR #4 on WordPress/wordpress.org by dd32.


5 days ago

This brings the recent addition of GitHub PR support on core.trac.wordpress.org to Meta.trac.wordpress.org

This is part of https://meta.trac.wordpress.org/ticket/4903

For more information on how this is supposed to work, check out https://make.wordpress.org/core/2020/02/21/working-on-trac-tickets-using-github-pull-requests/

#34 follow-up: @dd32
5 days ago

In 9695:

Trac: Enable GitHub PR support for Meta Trac.

See https://github.com/WordPress/wordpress.org/pull/4.
See #4903.

This ticket was mentioned in PR #5 on WordPress/wordpress.org by dd32.


5 days ago

This change causes the Travis CI status to be queried directly for determining the PR state, rather than relying upon the PR state variables.

Strangely the mergeable_state field is showing blocked for some PRs, where the Web and curl requests show clean.. After reviewing the PRs with that state, the new Travis checks will catch those otherwise they appear to be clean.

Part of https://meta.trac.wordpress.org/ticket/4903

#37 in reply to: ↑ 34 ; follow-up: @coffee2code
5 days ago

Replying to dd32:

Trac: Enable GitHub PR support for Meta Trac.

I've noticed that @otto42, @ryelle, and myself (at least amongst recent committers) don't have our commits associated with our GitHub accounts. Anything we'd need to do to make that connection? My username and email address is the same on both services.

#38 in reply to: ↑ 37 @ocean90
5 days ago

Replying to coffee2code:

Anything we'd need to do to make that connection? My username and email address is the same on both services.

You only have to add your-username@git.wordpress.org to https://github.com/settings/emails, the email doesn't need to be verified.

#39 @Otto42
5 days ago

That worked, can confirm.

#40 @coffee2code
5 days ago

Excellent. Thanks!

Note: See TracTickets for help on using tickets.