Skip to content

Add simplistic labeller#2662

Merged
pierotibou merged 9 commits into
masterfrom
pierre/simple-labeller
Apr 12, 2022
Merged

Add simplistic labeller#2662
pierotibou merged 9 commits into
masterfrom
pierre/simple-labeller

Conversation

@pierotibou

@pierotibou pierotibou commented Apr 9, 2022

Copy link
Copy Markdown
Contributor

Summary of changes

Adding a simplistic labeler that allows adding label when

  • PR title matches a string (eg [CIApp] or CIApp)
  • All modified files path matches a string (eg profiler/*)

Currently, this allows to add CIApp, Serverless, AppSec, Profiler and Debugger labels easily.
In a second commit, I've also added more labels taken from Lucas' PR (#2212) and in a third, modified the release notes to contain Serverless as well

Reason for change

Following #2607, we now categorize release notes based on labels, using the area-{component} label. So we thought we could revisit having a labeler. I spent a bit of time trying to find one already available but they do not match our needs (matching PR description, matching all files, not removing labels). So I decided to implement a simplistic one as we already have all the tools to do so.

Implementation details

Added an action, its configuration and a Nuke target.

Test coverage

Tested on my fork (mainly the first commit)

Next steps

We could imagine also:

  • Adding the type:bug when the PR summary Fixes section is filled
  • Split regexp on multiple lines for readability
  • Discuss with other tracer team to see if this target could interest them and make it a real action?

@pierotibou
pierotibou requested a review from a team as a code owner April 9, 2022 23:21
@github-actions github-actions Bot added the area:builds project files, build scripts, pipelines, versioning, releases, packages label Apr 9, 2022

@andrewlock andrewlock left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 👍 Feel free to ignore all the comments, I got carried away, and this will do the job as-is IMO 😂

Comment thread .github/labeller.yml
Comment on lines +23 to +24
- name: "area:builds"
allFilesIn: "\\.github\/.*|tracer\/build\/.*|\\.azure-pipelines\/.*|gitlab-ci\\.yml"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should remember to update this label if/when we move Nuke to be top-level

Comment thread .github/labeller.yml
Comment thread .github/labeller.yml Outdated
Comment thread .github/labeller.yml
Comment thread .github/labeller.yml
Comment thread .github/labeller.yml
@@ -0,0 +1,52 @@
labels:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO, if this is going to be ingested and converted in Nuke, we should just define them in Nuke, instead of having to parse and interpret from YAML. If you defined them in Nuke directly, you get extra IDE support for the regexes, and don't limit yourself to what can be defined in YAML. I don't think it gains a lot.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. I didit as a reflex to split conf and code. But I don't mind either way.

name: GitHubRepositoryName,
number: PullRequestNumber.Value);

GitTasks.Git("fetch origin master:master", logOutput: false);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should remove this line? Seems unnecessary given you've already done a checkout?

Suggested change
GitTasks.Git("fetch origin master:master", logOutput: false);
GitTasks.Git("fetch origin master:master", logOutput: false);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So my first try was without this line. And it failed fatal: ambiguous argument 'master': unknown revision or path not in the working tree..
This is me fixing the issue after a quick look at StackOverflow :p So I'd happily change to a better solution (or at least adding a comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ah, the next line should probably origin/master. The fetch here is creating a local master branch and associating it with the upstream, which you don't want/need to do 🙂

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot. Fixed in 11c3cb3

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually, it didn't work. I've roll backed and will merge :) I can revisit later

Comment thread .github/workflows/auto_label_prs.yml
Comment thread .github/labeller.yml
allFilesIn: "docs\/.*|.*README.*|.*\\.md"

- name: "area:installers"
allFilesIn: "shared\/src\/msi-installer\/.*"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would files here also receive the area:shared-components label below? Ideally I would only want this label and not both

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 74dd358

@pierotibou
pierotibou merged commit 56b251e into master Apr 12, 2022
@pierotibou
pierotibou deleted the pierre/simple-labeller branch April 12, 2022 06:52
@github-actions github-actions Bot added this to the vNext milestone Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:builds project files, build scripts, pipelines, versioning, releases, packages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants