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

CI: include automatic Pull Request Labeler #10094

Closed
wants to merge 1 commit into from

Conversation

pepe2k
Copy link
Member

@pepe2k pepe2k commented Jun 20, 2022

This adds GitHub CI action which makes use of 'Labeler', allowing automatic labeling of new PRs, based on the modified files paths.

Below labels are supported and more can be added later:

  • 'target/*'
  • 'target/imagebuilder'
  • 'kernel'
  • 'core packages'
  • 'build/scripts/tools'
  • 'toolchain'
  • 'GitHub/CI'

For more information: https://github.com/marketplace/actions/labeler

@pepe2k pepe2k added the GitHub/CI pull requests/issues for GitHub, CI and related stuff label Jun 20, 2022
@pepe2k pepe2k requested review from aparcar, adschm and ynezz June 20, 2022 14:07
@aparcar
Copy link
Member

aparcar commented Jun 20, 2022

Looks good, can we label it based on branch, too?

@pepe2k
Copy link
Member Author

pepe2k commented Jun 20, 2022

Looks good, can we label it based on branch, too?

Not with this one, it only analyzes paths of modified files.
But what exactly do you have in mind, labels for target version the PR is for?

@Ansuel
Copy link
Member

Ansuel commented Jun 21, 2022

I also add this idea... but my idea was to create a bot with nodejs and host it instead of using actions
The idea was having more control and a bot assigning label was better than an actions...
(and also that would permit us to introduce useful command like /merged that would automatically flag the pr as merged and provide the hash in the git repo)

@Ansuel
Copy link
Member

Ansuel commented Jun 21, 2022

Not with this one, it only analyzes paths of modified files.
But what exactly do you have in mind, labels for target version the PR is for?

Think he was referring to pr specific to openwrt release

@pepe2k
Copy link
Member Author

pepe2k commented Jun 21, 2022

@Ansuel I'm fine with anything else but this solution exists now so why not use it till we have something more sophisticated?

@Ansuel
Copy link
Member

Ansuel commented Jun 21, 2022

@pepe2k sure i wanted to put some comments just to get some feedback of my idea...
(ideally the bot should be hosted on github servers)

But yhea ok for me for using actions to autolabel :D

@pepe2k
Copy link
Member Author

pepe2k commented Jun 22, 2022

sure i wanted to put some comments just to get some feedback of my idea...
(ideally the bot should be hosted on github servers)

Do you have a working PoC or some similar solution available somewhere to look at?

@Ansuel
Copy link
Member

Ansuel commented Jun 22, 2022

@pepe2k today i wasted some time with a PoC... (don't judge the following link ahahahah)

https://github.com/Ansuel/test-openwrt-webook
The bot is currently running on my local system.

Anyway in practice my solution is a custom github app.
Needs to be hosted but permit more flexibility than simple actions...

Current implementation assume an issue with for example

ath79: something

where ath79 is parsed with a regex (everything before : is used), we search if a label exist with that and assign it.

I also tested command. If you send a comment with /thanks the bot send a specific message.

Tell me what do you think about this.

(currently this is only implemented for issue but can be expanded and it's only a PoC)

@Ansuel
Copy link
Member

Ansuel commented Jun 22, 2022

(i'm turning off the local server... put here a message and i will reopen it ASAP)

@Ansuel
Copy link
Member

Ansuel commented Jun 29, 2022

New on this? @aparcar @pepe2k
Is the bot hosting viable? The implementation is very simple and much more flexible than a CI action

@pepe2k
Copy link
Member Author

pepe2k commented Jun 29, 2022

@Ansuel

Needs to be hosted but permit more flexibility than simple actions...

And needs to be hosted/maintained by someone.
We should limit amount of additional services and work around the project and focus on important parts.

Current implementation assume an issue with for example

ath79: something

where ath79 is parsed with a regex (everything before : is used), we search if a label exist with that and assign it.

I'm focusing on PRs here only, not issues which are much more complicated and might require a sophisticated solution.

I think this might not be reliable. You assume contributors will follow our patch submitting guide and provide correct prefixes - believe me, that's not always true. Solution based on a paths modified by commit/s in a selected PR is just simple and human-error prone.

I also think a simple regex on PR/issue title for labeling can also be done with actions.

Cheers,
Piotr

@pepe2k pepe2k self-assigned this Jun 29, 2022
@Ansuel
Copy link
Member

Ansuel commented Jun 29, 2022

And needs to be hosted/maintained by someone.
We should limit amount of additional services and work around the project and focus on important parts.

This is my concern... Anyway the bot is so easy that once done it doesn't require that much changes...

I'm focusing on PRs here only, not issues which are much more complicated and might require a sophisticated solution.

The example was with issue just because it was easier to create tons of issue to develop the bot.
But it was just a proposal of a IMHO better solution

@pepe2k
Copy link
Member Author

pepe2k commented Jun 29, 2022

@Ansuel

This is my concern... Anyway the bot is so easy that once done it doesn't require that much changes...

Whenever I hear something like this, I run away 😏

But it was just a proposal of a IMHO better solution

I still fail to understand and see why it's better?

Cheers,
Piotr

@Ansuel
Copy link
Member

Ansuel commented Jun 29, 2022

I still fail to understand and see why it's better?

  • Can be extended (command to automate some process like invalid pr/issue., invalid Sob, add relevant commit hash merged in master...)
  • Can also be used with issue
  • Check tags relevant to branch
  • A bot that adds label instead of an action/a labeller from marketplace

@ynezz
Copy link
Member

ynezz commented Jun 30, 2022

This adds GitHub CI action which makes use of 'Labeler', allowing automatic labeling of new PRs, based on the modified files paths.

LGTM as an immediate solution.

My gripe of such solution is, that we need to have this file included in our source code repository, which is going to pollute Git log history with a commits which are not related to the source code and each such commit/modification then triggers a build pipeline on our buildbot and build firmware images for all targets (~24 hours), so wasting a lot of resources.

Well, this is probably just an issue with our workflow and tooling around, so we should probably just start ignoring sole commits to a predefined directories like .github on buildbot.

And needs to be hosted/maintained by someone.

FYI you can use scheduled GitHub Action for that purpose, which is essentially "serverless" cron job. I'm for example using such feature to self-assign patches on our patchwork, it's running every two hours and doesn't need any additional infrastructure.

You assume contributors will follow our patch submitting guide and provide correct prefixes - believe me, that's not always true.

You can enforce such formalities with GH Actions.

I'm focusing on PRs here only, not issues which are much more complicated and might require a sophisticated solution.

Well, on GitHub every pull request is an issue, but not every issue is a pull request, so they share a lot. Using templates to encourage useful issues and pull requests should help a lot as well.

BTW GitLab is using machine learning for a classification based on a issue content.

@Ansuel
Copy link
Member

Ansuel commented Jul 4, 2022

Also OK for me as a solution for now but IMHO we should use specialized tools instead of a simple labeler :D

@Ansuel
Copy link
Member

Ansuel commented Sep 5, 2022

@pepe2k any news on this?

@pepe2k
Copy link
Member Author

pepe2k commented Sep 7, 2022

@Ansuel I will rebase and look again into this in next days, thanks for reminder!

@aparcar
Copy link
Member

aparcar commented Sep 13, 2022

Looks good. I'd merge this

This adds GitHub CI action which makes use of 'Labeler', allowing
automatic labeling of new PRs, based on the modified files paths.

Below labels are supported and more can be added later:
- 'target/*'
- 'target/imagebuilder'
- 'kernel'
- 'core packages'
- 'build/scripts/tools'
- 'toolchain'
- 'GitHub/CI'

For more information:
https://github.com/marketplace/actions/labeler

Signed-off-by: Piotr Dymacz <pepe2k@gmail.com>
@pepe2k
Copy link
Member Author

pepe2k commented Sep 13, 2022

@aparcar done.
@ynezz thanks for all your comments,so far, I failed to find time to address them but will try again, this time harder (at least wasting buildbots resources is something I would like to look at)! :)

Hope it will do the job until we have something better

Cheers,
Piotr

@pepe2k pepe2k closed this Sep 13, 2022
@pepe2k pepe2k deleted the ci_pr-labeler branch September 14, 2022 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GitHub/CI pull requests/issues for GitHub, CI and related stuff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants