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

contributing: Add auto-labelling to pull requests #531

Merged
merged 8 commits into from
Dec 30, 2023

Conversation

nilason
Copy link
Contributor

@nilason nilason commented Apr 20, 2020

This adds auto-labelling to pull requests (related to #526).

It works with GitHub actions and add labels based on the PR's modified/added/deleted files location in repository.

This is a proposal, so if you find this approach appealing at all, I'm happy to receive suggestions for modifications.

Present proposal requires addition of some new labels.

The rules are stated in labeller.yml and as an example any modification in lib or include directory would add a lib label to the PR.

@landam landam requested review from neteler and wenzeslaus May 10, 2020 09:41
@landam landam added the enhancement New feature or request label May 10, 2020
.github/labeler.yml Outdated Show resolved Hide resolved
.github/labeler.yml Outdated Show resolved Hide resolved
.github/labeler.yml Show resolved Hide resolved
.github/labeler.yml Show resolved Hide resolved
.github/labeler.yml Outdated Show resolved Hide resolved
@wenzeslaus
Copy link
Member

I think this would be useful to have or at least try out in action even if it is limited e.g. to the currently existing labels (if we want to keep the number of labels low).

@neteler
Copy link
Member

neteler commented Jun 5, 2020

I think this would be useful to have or at least try out in action even if it is limited e.g. to the currently existing labels (if we want to keep the number of labels low).

We may try, otherwise we'll never find out...

@neteler
Copy link
Member

neteler commented Jul 25, 2020

@nilason does this PR need to be updated since GH Actions are now in place?

@nilason
Copy link
Contributor Author

nilason commented Jul 25, 2020

@nilason does this PR need to be updated since GH Actions are now in place?

No, this is a separate feature, should work as is.

Copy link
Member

@neteler neteler left a comment

Choose a reason for hiding this comment

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

Some comments added inline...

@nilason nilason force-pushed the add-pull-request-auto-labelling branch from af65037 to dcf85d2 Compare December 9, 2021 17:38
@nilason
Copy link
Contributor Author

nilason commented Dec 9, 2021

Rebased and updated to better reflect current list of labels.

The labels libraries and module (or alternative label) need to be created for this to work.

I commented out docs and manual, for the time being.

@nilason nilason added this to the 8.2.0 milestone Dec 9, 2021
@nilason nilason changed the title WIP: Add auto-labelling to pull requests, proposal Add auto-labelling to pull requests Dec 11, 2021
@wenzeslaus wenzeslaus modified the milestones: 8.2.0, 8.4.0 Feb 27, 2022
@wenzeslaus wenzeslaus modified the milestones: 8.3.0, 8.4.0 Feb 10, 2023
@neteler neteler force-pushed the add-pull-request-auto-labelling branch from 3d11ad8 to dcf85d2 Compare November 7, 2023 13:26
@neteler
Copy link
Member

neteler commented Nov 7, 2023

PR rebased, but it needs more permissions:

Run actions/labeler@v4
...
Warning: The action requires write permission to add labels to pull requests. For more information please refer to the action documentation: https://github.com/actions/labeler#permissions
Error: Resource not accessible by integration

https://github.com/actions/labeler#permissions

@nilason
Copy link
Contributor Author

nilason commented Nov 7, 2023

PR rebased, but it needs more permissions:

Run actions/labeler@v4
...
Warning: The action requires write permission to add labels to pull requests. For more information please refer to the action documentation: https://github.com/actions/labeler#permissions
Error: Resource not accessible by integration

https://github.com/actions/labeler#permissions

Thanks!

Updated, works now on a private test repo of mine. (Looks like it have to be merged before kicking in).

.github/labeler.yml Show resolved Hide resolved
.github/labeler.yml Show resolved Hide resolved
.github/labeler.yml Outdated Show resolved Hide resolved
.github/labeler.yml Outdated Show resolved Hide resolved
.github/labeler.yml Outdated Show resolved Hide resolved
.github/labeler.yml Outdated Show resolved Hide resolved
@wenzeslaus
Copy link
Member

Updated, works now on a private test repo of mine.

I don't see any permissions in the YAML file, but I see them in the example in doc linked by @neteler. Any insights? What I'm asking is basically, do you expect the workflow to successfully run after merging it with the current configuration in terms of permissions?

(Looks like it have to be merged before kicking in).

That's definitively the case.

@nilason
Copy link
Contributor Author

nilason commented Nov 7, 2023

Updated, works now on a private test repo of mine.

I don't see any permissions in the YAML file, but I see them in the example in doc linked by @neteler. Any insights? What I'm asking is basically, do you expect the workflow to successfully run after merging it with the current configuration in terms of permissions?

I changed to pull_request_target as was suggested by https://github.com/actions/labeler#permissions. Then, yes, I expect it to work. The reason for this permission issue, is that non-existing labels are automatically created. If this was possible (with pull_request) anyone can create (maliciously) an x number of labels (only by submitting a PR).

(Looks like it have to be merged before kicking in).

That's definitively the case.

Yep.

@nilason nilason added the CI Continuous integration label Nov 27, 2023
Copy link
Member

@echoix echoix left a comment

Choose a reason for hiding this comment

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

I left some new suggestions to have this PR up to date with changes with the action used. I would really like this PR to come through soon, it would be quite useful for tidying up the backlog (that I started doing)

.github/workflows/label.yml Outdated Show resolved Hide resolved
.github/workflows/label.yml Outdated Show resolved Hide resolved
.github/workflows/label.yml Show resolved Hide resolved
.github/workflows/label.yml Show resolved Hide resolved
.github/workflows/label.yml Outdated Show resolved Hide resolved
.github/workflows/label.yml Outdated Show resolved Hide resolved
@echoix
Copy link
Member

echoix commented Dec 22, 2023

@neteler Just so you know, in this section it mentions that the action might fail one day at the time of a major version update of the action that would introduce breaking changes in the config, so that we might need to force it through while using the pull_request_target event type. That event type is really the most appropriate here however. They explain the workaround if ever needed.
https://github.com/actions/labeler#updating-major-version-of-the-labeler

@echoix
Copy link
Member

echoix commented Dec 22, 2023

And for the different labels, and the old discussions about various small details, I think that it's overall quite adequate, docs is a good idea, since having details on html, css, JavaScript might be overkill.

However, if is possible to make a distinction between docs, the content, and docs, the system/infrastructure around it, that might be a better solution than having things like "manual".

Docs-only changes should be able to be merged more quickly without the same amount of programming-level code review. My stance is that it is already both hard to have contributors in open source and hard to have good docs in open source, so let's not leave out the contributions we could have. If we consider that documentation changes is often the first point of entry in contributing in open source, its even more important to offer a good experience to start with. Thus, when a PR would have docs (the content) only changes, if it's clearer and no mistakes, it could be merged expeditiously. Docs, as the infrastructure, could be like any other topic, and I think there is work on changing from manually creating HTML to markdown, so these extra categories will change in the mid-future.

In conclusion, this PR, as a tool for us, should be fine tuned after usage, and it's our (changing) usage that dictates its requirements. We can't be that wrong with this. Our irritants will show us what to improve.

@echoix
Copy link
Member

echoix commented Dec 30, 2023

Do I have the blessing to apply the suggestions I think are needed to unblock this PR?

@nilason
Copy link
Contributor Author

nilason commented Dec 30, 2023

Do I have the blessing to apply the suggestions I think are needed to unblock this PR?

I'm sitting with it at this moment.

@nilason
Copy link
Contributor Author

nilason commented Dec 30, 2023

Thanks @echoix for the suggestions, added them with new commit.

@nilason
Copy link
Contributor Author

nilason commented Dec 30, 2023

May I suggest to merge this ASAP, this workflow may be fine-tuned or even dropped is not good enough.

@echoix
Copy link
Member

echoix commented Dec 30, 2023

I was about to finish approving it and go for merging. Merging main into the PR branch wasn't explicitly needed. We have enabled the option to always show it, so it was a suggestion rather than a warning, and it helps to retrigger CI for older PRs, without having to close/open the PR (which would run the same outdated CI), and rather pull in some new code that might include CI changes.

We'll see in 1h30+ if we can merge it then..

@echoix echoix dismissed stale reviews from neteler and wenzeslaus December 30, 2023 21:23

Discussion resolved now, and further tuning will be done at a later time

@echoix echoix removed the request for review from wenzeslaus December 30, 2023 21:24
@echoix echoix merged commit 6b4c665 into OSGeo:main Dec 30, 2023
20 checks passed
@nilason nilason deleted the add-pull-request-auto-labelling branch January 2, 2024 07:38
HuidaeCho pushed a commit to HuidaeCho/grass that referenced this pull request Jan 9, 2024
* GitHub: add auto-labelling to pull requests

* add Markdown; add python/; update action version

* use pull_request_target

* update after review feedback

* use docs label for docs

* update for actions/labeler@v5 with added review suggestions

---------

Co-authored-by: Markus Neteler <neteler@gmail.com>
Co-authored-by: Edouard Choinière <27212526+echoix@users.noreply.github.com>
@wenzeslaus wenzeslaus changed the title Add auto-labelling to pull requests contributing: Add auto-labelling to pull requests Jun 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous integration enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants