KAFKA-17542: Use actions/labeler for automatic PR labeling - part 2

[TaiJuWu]

Jira: https://issues.apache.org/jira/browse/KAFKA-17542
test on: TaiJuWu#11

Committer Checklist (excluded from commit message)

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

[chia7712]

@TaiJuWu thanks for this patch. what about the plan of "tests" label?

[mumrah]

@TaiJuWu, thanks for following up with this patch!

Can we create this as a standalone bash script that lives in .github/scripts? I don't like having too much code directly in the workflow definition. This also makes it easier to test locally.

The script inputs (the SHAs to compare) can be env variables. If we need outputs, we can use the stdout "FOO=BAR" notation (see https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#setting-an-output-parameter).

A few other considerations:

1. Should we have other size labels? E.g., "tee-shirt" sizes: small, medium, large, xlarge
2. If a PR's size changes after an update, we may need to remove an existing label
3. Do we need to exclude any files from this diff sizing? I can't think of any...

[mumrah]

Not sure if it helps, but we can use the gh CLI here.

https://cli.github.com/manual/gh_pr_diff

[TaiJuWu]

Thanks. It is very useful.

[TaiJuWu]

@TaiJuWu thanks for this patch. what about the plan of "tests" label?

Hi @chia7712 , the test label are supported in last PR here
I think it meets your requirement.

[TaiJuWu]

@TaiJuWu, thanks for following up with this patch!

Can we create this as a standalone bash script that lives in .github/scripts? I don't like having too much code directly in the workflow definition. This also makes it easier to test locally.

Sure.

The script inputs (the SHAs to compare) can be env variables. If we need outputs, we can use the stdout "FOO=BAR" notation (see https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#setting-an-output-parameter).

Thanks for information!

A few other considerations:

1. Should we have other size labels? E.g., "tee-shirt" sizes: small, medium, large, xlarge
2. If a PR's size changes after an update, we may need to remove an existing label
3. Do we need to exclude any files from this diff sizing? I can't think of any...

I completely agree. To support multiple size labels, we can use PR-size-labeler. However, a limitation is that it requires us to set exactly five labels.

[mumrah]

To support multiple size labels, we can use PR-size-labeler

Please note that while we can use marketplace actions, we must have them approved by ASF Infra team unless it's from GitHub or a verified developer. I think this is a simple enough action that we can roll our own, plus it gives us more flexibility for the labels (e.g., we can start with just "small")

[apoorvmittal10]

test on: TaiJuWu#10

I don't find any changes as per this PR on mentioned test PR, am I missing something?

[TaiJuWu]

test on: TaiJuWu#10

I don't find any changes as per this PR on mentioned test PR, am I missing something?

Hi @apoorvmittal10 , thanks for review.
This tes PR only shows the action can label mirror automatically as below.

By the way, we may use another tool help us to finish this target, you can refere here

[TaiJuWu]

To support multiple size labels, we can use PR-size-labeler

Please note that while we can use marketplace actions, we must have them approved by ASF Infra team unless it's from GitHub or a verified developer. I think this is a simple enough action that we can roll our own, plus it gives us more flexibility for the labels (e.g., we can start with just "small")

I file a jira for this https://issues.apache.org/jira/browse/INFRA-26158

[mumrah]

@TaiJuWu I thought we were going to stick with your script for this? The PR-size-labeler is going to add a size label to every PR, which I'm not sure we want. I like the approach you started in this PR for just labeling "minor" PRs

[TaiJuWu]

@TaiJuWu I thought we were going to stick with your script for this? The PR-size-labeler is going to add a size label to every PR, which I'm not sure we want. I like the approach you started in this PR for just labeling "minor" PRs

Oh, sorry for the misunderstanding.
If we only need a mirror label, we will have to write our own script, as the PR-size-labeler cannot be used in this case.
I'm also not entirely sure which solution is the best for us. We can start with just the mirror and consider using the PR-size-labeler in the future if necessary.

[mumrah]

Oh, sorry for the misunderstanding.

No worries 😄 . I think we can start with your script and update it as needed.

So, to finish this up, we need to

1. Move the bash into a script
2. Change the label name to minor (not mirror)
3. Address @chia7712's question about the "test" label. If this is too complex we can create a new JIRA for it and work on it later

[TaiJuWu]

Oh, sorry for the misunderstanding.

No worries 😄 . I think we can start with your script and update it as needed.

So, to finish this up, we need to

1. Move the bash into a script
2. Change the label name to minor (not mirror)
3. Address @chia7712's question about the "test" label. If this is too complex we can create a new JIRA for it and work on it later

Hi @mumrah ,
[1] and [2] are finished. I will check [3] with @chia7712 to ensure that the current test label is functioning properly.

[mumrah]

Thanks for the updates, the script looks good.

I think we should considered a different label than minor. After all, we may have very serious PRs which only have a few changes :). "Minor" can also convey importance or significance. I suggest we go with "small" which is more accurate.

[TaiJuWu]

Thanks for the updates, the script looks good.

I think we should considered a different label than minor. After all, we may have very serious PRs which >only have a few changes :). "Minor" can also convey importance or significance. I suggest we go with >"small" which is more accurate.

Thanks for review and suggestion, updated!

[mumrah]

LGTM, thanks @TaiJuWu!

[chia7712]

@TaiJuWu thanks for this useful feature!

[chia7712]

It seems that printf tries to parse the content of pr_diff, which may lead to invalid formats (see #17338).

Perhaps we should avoid manually parsing the content and use gh pr view instead. For example:

pr_diff=$(gh pr view $PR_NUM -R $GITHUB_REPOSITORY --json additions,deletions)

insertions=$(echo "$pr_diff" | jq -r '.insertions')
deletions=$(echo "$pr_diff" | jq -r '.deletions')

total_changes=$((insertions + deletions))

@TaiJuWu WDYT? could you open a MINOR to fix it?

[TaiJuWu]

Thanks for your remind, will update.