-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: add PR labeler #4205
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
feat: add PR labeler #4205
Conversation
I've got nothing against this, but do we really have the volume to need it? What would we use the classifications for? We don't have multiple squads or anything... |
It's just nice to have. There is no real importance to it I agree. I tend to add these labels manually and recently found it annoying, so I figured why not automate it away. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As good a reason as any
.github/labeler.yml
Outdated
gcs: | ||
- registry/storage/gcs | ||
|
||
azure: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the manually created ones by us. Labeler action will create new ones but it's OK IMHO. We can always clean up the old cruft.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the area/xx
prefixes, as they are clear on what "dimension" the label is for. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok will change, sir 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I've updated them all. Now the labeler will use all the existing labels and they have area
prefix 😄
.github/labeler.yml
Outdated
vendor: | ||
- vendor/**/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably use the dependencies
label (and we should remove the area/vendor
label, but first relabel existing PRs if it's used)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this could be changed to go.mod
instead of the vendor
dir 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was considering adding those, but also thought changing those should also modify the vendor directory, so perhaps not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've actually added both go.mod
and go.sum
to this matcher. It does no harm 😄
Having the classifications can really help finding back PR's and tickets when the list of PR's and tickets grows. GitHub's search isn't great (far from), so only searching for keywords often becomes problematic. Combining keywords with specific labels can help narrow down results. In addition we can consider using these labels for generating release notes. For moby/moby, we also have
|
Updated PTAL @thaJeztah |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
(you know what's coming; can you squash the commits 😅 ?)
.github/labeler.yml
Outdated
area/config: | ||
- charts/registry/config/**/* | ||
|
||
documentation: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could rename the existing label to also be area/docs
, but we can do in a follow-up if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
.github/labeler.yml
Outdated
@@ -0,0 +1,50 @@ | |||
GHA: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh! missed this one; looks like we have area/ci
; that's worth updating before merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
Will squash and then merge. |
This is an initial commit to kickstart a conversation about how we want the new PRs to be labeled. TBC. Signed-off-by: Milos Gajdos <milosthegajdos@gmail.com>
c921067
to
e96fce1
Compare
OMG this is stuck on the DCO. I'm merging, because the commit is signed and Gh need to fix their cruft. |
This PR adds a labeler action that automatically adds labels to PRs based on matched paths (see here).
We can do some quite elaborate things but this is an initial PR to kickstart a conversation about how we want the new PRs to be labelled. TBC.
PTAL @thaJeztah