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

[DO NOT MERGE THIS PR] Add slash command "/black" to format PRs #645

Closed
wants to merge 5 commits into from

Conversation

seisman
Copy link
Member

@seisman seisman commented Oct 9, 2020

Description of proposed changes

This PR adds a special github action to enable slash commands in comments. With the feature enabled, anyone, who have written permission to a pull request, can write /black at the first line of a comment to trigger the "black" action, which can format the codes automatically.

I tried to write the slash command /black in the comments below, but it doesn't work as I expect. It seems the special event "repository dispatch" can only read configurations files in the master branch. Thus, testing the action will definitely pollute the git history of the master branch. So I forked pygmt to my account (https://github.com/seisman/pygmt) and do some tests on my own fork.


The slash command works well in my fork. Here are the changes to the master branch in my own fork: master...seisman:master (I will open a PR to merge these changes into master branch).

https://github.com/seisman/pygmt/pull/5 is a PR I opened with some bad changes to codes. Writting /format (I use /format instead of /black in my fork) in the comment triggered the action to format the codes automatically.

Here is the screenshot:
image


Please review the changes of master...seisman:master (I made a PR in #646 for easier review).

BTW, feel free to submit PRs to my fork https://github.com/seisman/pygmt, to check if slash commands work for PRs from external contributors.

NOTE: PR #645 won't be merged.

@seisman
Copy link
Member Author

seisman commented Oct 9, 2020

/black

Edit4

@seisman seisman marked this pull request as ready for review October 9, 2020 03:20
@seisman
Copy link
Member Author

seisman commented Oct 9, 2020

/black

@seisman seisman marked this pull request as draft October 9, 2020 04:10
@seisman seisman requested a review from weiji14 October 9, 2020 06:12
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Thanks for doing this @seisman, it will be a really useful addition to the project! I know just how much trial and error setting up this sort of Github Action can take, but it's looking promising.

Just one comment about the PAT for now. Later on, once we settle on the implementation, it will be best to document this somewhere too.

- name: Add reaction
uses: peter-evans/create-or-update-comment@v1
with:
token: ${{ secrets.PAT }}
Copy link
Member

@weiji14 weiji14 Oct 9, 2020

Choose a reason for hiding this comment

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

Whose Personal Access Token (PAT) is this, and what scope does it have? I see it's stored as a secret in the PyGMT repo (though I can't see the secret itself), and know that it's needed to let the Github Action 'bot' do the work. Just asking because I realize at conda-forge/gmt-feedstock#100 (comment) that we might need to use a PAT there too. Was wondering if we could have a GMT organization-wide secret PAT for these Github Actions that require permission to make commits.

Alternatively, we could look into using https://github.com/tibdex/github-app-token - i.e. make an organization GMT Github App, and use that App to generate what is basically a PAT token on the fly (rather than one of our personal ones). See also https://github.com/peter-evans/create-pull-request/blob/master/docs/concepts-guidelines.md#authenticating-with-github-app-generated-tokens for instructions on how to set this up. I'd help out with this but haven't got admin permissions in the GMT organization 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Whose Personal Access Token (PAT) is this, and what scope does it have?

It's a PAT of my own account, with the repo scope.

Was wondering if we could have a GMT organization-wide secret PAT for these Github Actions that require permission to make commits.

It seems impossible to have an organization-wide PAT. The only way is someone generates a PAT and saves it as an organization-wide secret.

Alternatively, we could look into using https://github.com/tibdex/github-app-token - i.e. make an organization GMT Github App, and use that App to generate what is basically a PAT token on the fly (rather than one of our personal ones).

Yes, it looks promising.

I'd help out with this but haven't got admin permissions in the GMT organization.

Perhaps you can ask for admin permissions if necessary.


BTW, this PR doesn't work. The working one is PR #646.

Copy link
Member

@weiji14 weiji14 Oct 9, 2020

Choose a reason for hiding this comment

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

Ok, shall we try using tibdex/github-app-token then? The step by step instructions at https://github.com/peter-evans/create-pull-request/blob/master/docs/concepts-guidelines.md#authenticating-with-github-app-generated-tokens seem fairly straightforward. After we make the GMT-bot Github App, we can 'install' it here on GenericMappingTools/pygmt and at GenericMappingTools/gmt-feedstock to authenticate these bots.

And yes, I'll review things on #646 later.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe hold on for a while, Github is experiencing some degraded performance (see https://www.githubstatus.com/incidents/28js6274ybpb. Noticed this in one of my other PRs.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems that the github incident is resolved, right?

@seisman seisman changed the title Add slash command "/black" to format PRs [DO NOT MERGE THIS PR] Add slash command "/black" to format PRs Oct 21, 2020
@seisman seisman closed this Oct 24, 2020
@seisman seisman deleted the slash-format branch October 24, 2020 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants