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

KAFKA-17542: Use actions/labeler for automatic PR labeling #17208

Open
wants to merge 12 commits into
base: trunk
Choose a base branch
from

Conversation

TaiJuWu
Copy link
Contributor

@TaiJuWu TaiJuWu commented Sep 16, 2024

Jira: https://issues.apache.org/jira/browse/KAFKA-17542

test on TaiJuWu#5

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

Copy link
Contributor

@mumrah mumrah left a comment

Choose a reason for hiding this comment

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

raft and metadata aren't labels we currently have. You can check the existing ones here: https://github.com/apache/kafka/labels

.github/workflows/labeler.yml Outdated Show resolved Hide resolved
.github/workflows/labeler.yml Show resolved Hide resolved
@mumrah mumrah added the build Related to the Github or Jenkins builds label Sep 16, 2024
@TaiJuWu
Copy link
Contributor Author

TaiJuWu commented Sep 16, 2024

raft and metadata aren't labels we currently have. You can check the existing ones here: https://github.com/apache/kafka/labels

Hi @mumrah , thanks for your review and info!

@chia7712
Copy link
Contributor

@TaiJuWu thanks for this patch I believe it will be an important feature to our PR flow. I list some brainstorming below. PTAL

  1. add module labels according to changed files
  2. add build/ci label if build.gradle/.github related files are changed
  3. add test label if only test code gets changed
  4. add minor label if there are few line changes
  5. add docs label if only docs get changed

Copy link
Contributor

@mumrah mumrah left a comment

Choose a reason for hiding this comment

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

Left some additional comments, @TaiJuWu.

I like @chia7712's suggestions. For 3 and 4, we may probably will need to do something more complex than simply using the labeler action. For example,

  1. add test label if only test code gets changed

will require getting the diff of the PR and checking the file paths. This will require a script of some kind.

.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
.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
Copy link
Contributor

@mumrah mumrah left a comment

Choose a reason for hiding this comment

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

Thanks @TaiJuWu, looks good. One minor suggestion.

If we want to wait on @chia7712's more complex suggestions (i.e., "minor" label and "tests" only PRs), we can do those in a part 2 PR.

.github/workflows/labeler.yml Outdated Show resolved Hide resolved
.github/labeler.yml Outdated Show resolved Hide resolved
@TaiJuWu
Copy link
Contributor Author

TaiJuWu commented Sep 18, 2024

Left some additional comments, @TaiJuWu.

I like @chia7712's suggestions. For 3 and 4, we may probably will need to do something more complex than simply using the labeler action. For example,

  1. add test label if only test code gets changed

will require getting the diff of the PR and checking the file paths. This will require a script of some kind.

Yes, you are right, but the action/labeler does not support scripts. I am still investigating how to achieve this goal.
Thanks for your review.

@TaiJuWu TaiJuWu marked this pull request as ready for review September 18, 2024 01:17
@mumrah
Copy link
Contributor

mumrah commented Sep 18, 2024

Yes, you are right, but the action/labeler does not support scripts

We can add an additional step to evaluate the diff and determine what labels to apply. We can use octokit or gh api (my preference) to actually set the label.

@TaiJuWu
Copy link
Contributor Author

TaiJuWu commented Sep 18, 2024

Thanks @TaiJuWu, looks good. One minor suggestion.

If we want to wait on @chia7712's more complex suggestions (i.e., "minor" label and "tests" only PRs), we can do those in a part 2 PR.

Yes, you are right, but the action/labeler does not support scripts

We can add an additional step to evaluate the diff and determine what labels to apply. We can use octokit or gh api (my preference) to actually set the label.

Ok. I am happy to handle this.
Another question, does we need our bot (like apache-bot) intead of github bot if we need to use gh api ?

@mumrah
Copy link
Contributor

mumrah commented Sep 18, 2024

Here is an example of using the gh tool within our workflows

https://github.com/apache/kafka/blob/trunk/.github/actions/gh-api-update-status/action.yml

It would be nice to have a reusable action similar to this for adding labels

@TaiJuWu
Copy link
Contributor Author

TaiJuWu commented Sep 18, 2024

Here is an example of using the gh tool within our workflows

https://github.com/apache/kafka/blob/trunk/.github/actions/gh-api-update-status/action.yml

It would be nice to have a reusable action similar to this for adding labels

Got it. Thanks a lot.

.github/labeler.yml Outdated Show resolved Hide resolved
.github/labeler.yml Outdated Show resolved Hide resolved
- changed-files:
- any-glob-to-any-file:
- 'clients/src/main/java/org/apache/kafka/clients/producer/**'

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add generator label?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, transaction may need a individual label too.

- changed-files:
- any-glob-to-any-file:
- 'group-coordinator/**'
- 'coordinator-common/**'
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add share-related folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @chia7712 ,
All comments are addressed in 80bfa4a
There is a special case share-coordinator, should we put share or coordinator to it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, we should use KIP-932 for the share coordinator

.github/labeler.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@TaiJuWu thanks for this patch!

Copy link
Contributor

@mumrah mumrah left a comment

Choose a reason for hiding this comment

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

edit: see below

Copy link
Contributor

@mumrah mumrah left a comment

Choose a reason for hiding this comment

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

Actually found a few issues remaining. Can you address these and then I'll merge. We don't need this to be perfect -- just a good starting point 😄

People focusing on specific project areas will be free to modify this config to auto-label things as they see fit.

.github/configs/labeler.yml Outdated Show resolved Hide resolved
.github/configs/labeler.yml Outdated Show resolved Hide resolved
.github/configs/labeler.yml Show resolved Hide resolved
.github/configs/labeler.yml Outdated Show resolved Hide resolved
@TaiJuWu
Copy link
Contributor Author

TaiJuWu commented Sep 19, 2024

Hi @mumrah , all comments are addressed, thanks for review.

Copy link
Contributor

@mumrah mumrah left a comment

Choose a reason for hiding this comment

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

last nit 😄

- any-glob-to-any-file:
- 'generator/**'

transaction:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: transactions (plural)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Related to the Github or Jenkins builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants