Skip to content

Conversation

milosgajdos
Copy link
Member

@milosgajdos milosgajdos commented Dec 20, 2023

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

@Jamstah
Copy link
Collaborator

Jamstah commented Dec 20, 2023

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...

@milosgajdos
Copy link
Member Author

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.

Copy link
Collaborator

@Jamstah Jamstah left a 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

gcs:
- registry/storage/gcs

azure:
Copy link
Member

Choose a reason for hiding this comment

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

Is this the label that is applied? (so if registry/storage/azure is touched, we apply azure as label?)

Because we have existing area/xxx labels; I think it would make sense to use those (same for various other ones);

Screenshot 2023-12-20 at 17 15 01

Copy link
Member Author

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.

Copy link
Member

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. 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok will change, sir 😄

Copy link
Member Author

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 😄

Comment on lines 22 to 23
vendor:
- vendor/**/*
Copy link
Member

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)

Copy link
Member Author

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 🤔

Copy link
Member

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.

Copy link
Member Author

@milosgajdos milosgajdos Dec 20, 2023

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 😄

@thaJeztah
Copy link
Member

What would we use the classifications for? We don't have multiple squads or anything...

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

  • kind/ labels (kind/bugfix, kind/enhancement etc) which (combined with area) can serve similar purposes
  • impact/ labels (impact/deprecation, impact/changelog, impact/api) which can help finding changes that are note-worthy (for mentioning in release-notes)

@milosgajdos
Copy link
Member Author

Updated PTAL @thaJeztah

Copy link
Member

@thaJeztah thaJeztah left a 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 😅 ?)

area/config:
- charts/registry/config/**/*

documentation:
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

@@ -0,0 +1,50 @@
GHA:
Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

@milosgajdos
Copy link
Member Author

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>
@milosgajdos
Copy link
Member Author

OMG this is stuck on the DCO. I'm merging, because the commit is signed and Gh need to fix their cruft.

@milosgajdos milosgajdos merged commit 012adca into distribution:main Dec 22, 2023
@milosgajdos milosgajdos deleted the add-labeler branch December 22, 2023 09:35
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.

3 participants