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-17606: Include Rat errors in GitHub workflow summary #17280

Merged
merged 7 commits into from
Oct 1, 2024

Conversation

LoganZhuZzz
Copy link
Contributor

In the current GitHub workflow, it has become difficult to locate Rat errors when running check -x test. To improve readability and make it easier to identify Rat errors, we need to include them in the workflow summary.

Key Modifications

  • Added a call to rat.py in the build.yml file to annotate Rat errors after the Compile and validate step.
  • Ensured that the annotation for Rat reports will execute only when the Compile and validate step fails.

Purpose

  • Enhance the usability of the GitHub workflow, allowing developers to more easily identify and fix Rat errors.
  • Improve workflow efficiency by reducing the time spent manually searching for errors.

Notes

  • Please ensure that all relevant workflow tests are run before merging this PR to confirm that the new feature operates correctly.

Committer Checklist (excluded from commit message)

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

@github-actions github-actions bot added the build Gradle build or GitHub Actions label Sep 26, 2024
@LoganZhuZzz
Copy link
Contributor Author

@chia7712 please take a look,thanks!

Copy link
Member

@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 for the patch @LoganZhuZzz!

Left a few minor comments inline. Can you temporarily add a couple of files without a license so we can see the script working in this PR?

@@ -68,6 +68,11 @@ jobs:
./gradlew --build-cache --info \
${{ inputs.is-public-fork && '--no-scan' || '--scan' }} \
check -x test
- name: Annotate Rat errors
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this down with the checkstyle step?

@@ -68,6 +68,11 @@ jobs:
./gradlew --build-cache --info \
${{ inputs.is-public-fork && '--no-scan' || '--scan' }} \
check -x test
- name: Annotate Rat errors
if: ${{ failure() }}
Copy link
Member

Choose a reason for hiding this comment

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

We should only run this on java 21 to avoid duplicate reports from the parallel builds

…errors' and restrict to Java 21 builds

- Moved the 'Annotate Rat errors' step to follow the 'Annotate checkstyle errors' step, ensuring both error annotation steps are grouped for better workflow clarity.
- Added a conditional check to only run the 'Annotate Rat errors' step on Java 21 to prevent duplicate reports across parallel builds, in line with the checkstyle step behavior.
- Temporarily added test files without licenses to validate that unapproved licenses are correctly summarized in the GitHub workflow, per feedback.
@LoganZhuZzz
Copy link
Contributor Author

LoganZhuZzz commented Sep 27, 2024

Thanks for the patch @LoganZhuZzz!

Left a few minor comments inline. Can you temporarily add a couple of files without a license so we can see the script working in this PR?

Thank you for the feedback on the patch @mumrah
I have made the following changes:

  1. Temporarily added test files without licenses to validate the unapproved licenses summary in the GitHub workflow.
  2. Moved the Annotate Rat errors step below the Annotate checkstyle errors step for better clarity.
  3. Added a version check to ensure annotation steps run only on Java 21 to prevent duplicate reports.

Please let me know if there’s anything else you would like me to address!

@LoganZhuZzz
Copy link
Contributor Author

1727412847728

This pull request successfully integrates the annotation of Rat errors into the GitHub workflow summary.

@mumrah
Copy link
Member

mumrah commented Sep 27, 2024

Thanks @LoganZhuZzz, the script output looks good. Can you try the following for me:

  1. Add a ignoreRatFailure parameter to build.gradle that controls rat { failOnError }
  2. Pass in this param in build.yml when we invoke check -x test
  3. Modify rat.py to exit 1 if it finds any problems

This way, the link to the failed workflow will take us to the Rat output rather than the first failing step.

Also, could you include "::notice" output in the script similar to this https://github.com/apache/kafka/blob/trunk/.github/scripts/checkstyle.py#L64? This is how we actually create the annotations. Note that the line number starts with 1. https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#setting-a-notice-message

- Added `ignoreRatFailure` parameter to `build.gradle` to control the `failOnError` behavior of the Rat task.
- Updated `build.yml` to pass the `ignoreRatFailure` parameter when invoking `check -x test`.
- Modified `rat.py` to exit with code 1 if unapproved licenses are found, ensuring that the workflow failure links directly to the Rat output for improved traceability.

This change facilitates better handling of Rat reports within the CI/CD pipeline.
@LoganZhuZzz
Copy link
Contributor Author

Thanks @LoganZhuZzz, the script output looks good. Can you try the following for me:

  1. Add a ignoreRatFailure parameter to build.gradle that controls rat { failOnError }
  2. Pass in this param in build.yml when we invoke check -x test
  3. Modify rat.py to exit 1 if it finds any problems

This way, the link to the failed workflow will take us to the Rat output rather than the first failing step.

Also, could you include "::notice" output in the script similar to this https://github.com/apache/kafka/blob/trunk/.github/scripts/checkstyle.py#L64? This is how we actually create the annotations. Note that the line number starts with 1. https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#setting-a-notice-message

Thanks for the guidance. @mumrah
I have fixed the above issues, If you notice any areas for improvement or any bad practices, please let me know.

Copy link
Member

@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 for the updates @LoganZhuZzz! Just a few small comments.

Can you try this out on your fork and include a link to the results?

build.gradle Outdated
@@ -210,6 +210,10 @@ def determineCommitId() {
apply from: file('wrapper.gradle')

if (repo != null) {
ext {
ignoreRatFailure = project.hasProperty('ignoreRatFailure') ? project.ignoreRatFailure.toBoolean() : false
Copy link
Member

Choose a reason for hiding this comment

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

Can you relocate this with other property handling? (Along with userMaxForks, etc)

@@ -67,7 +67,7 @@ jobs:
run: |
./gradlew --build-cache --info \
${{ inputs.is-public-fork && '--no-scan' || '--scan' }} \
check -x test
check -x test -PignoreRatFailure=true
Copy link
Member

Choose a reason for hiding this comment

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

Can you move the property before the task on a separate line?

rel_path = os.path.relpath(file, workspace_path)
print(f"::notice file={rel_path},title=Unapproved License::File with unapproved license")

exit(1 if total_unapproved_licenses > 0 else 0)
Copy link
Member

Choose a reason for hiding this comment

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

Can you make this a regular if/else rather than inlined?

- Moved the `ignoreRatFailure` property to a separate line before `check -x test` for clarity.
- Added a comment for the `-PignoreRatFailure=true` flag to improve documentation.
- Relocated `ext` block configuration for better management alongside
  other properties
@LoganZhuZzz
Copy link
Contributor Author

LoganZhuZzz commented Sep 29, 2024

Hi David @mumrah ! Here are the results on my fork:

Compile and Validate Results

b8d38b77eb24bd5773c4949083d5e2c

Annotate Rat Errors Results

09c6fe160eb0abde9296ce9920b0fb3

Link to Results


Questions

  1. After modifying the Annotate Rat errors step from if: ${{ failure() && matrix.java == '21' }} to if: ${{ matrix.java == '21' }}, the Annotate Rat errors step was able to execute. Is this behavior in line with our expectations?
  2. Do the results from Annotate Rat errors meet our expectations?

Root Cause of Annotate Rat Errors Step Not Executing

The reason for this issue is that in the Compile and validate phase, we execute:

./gradlew --build-cache --info \
${{ inputs.  is-public-fork && '--no-scan' || '--scan' }} \
-PignoreRatFailure=true \
check -x test

When the ignoreRatFailure parameter is set to true, the failOnError of the Gradle RAT task is set to false.This causes the failure() condition in the Annotate Rat errors step of the workflow (build.yml) to evaluate as false, preventing the Annotate Rat errors step from being executed. Does this align with what we expect?

@mumrah
Copy link
Member

mumrah commented Sep 29, 2024

When the ignoreRatFailure parameter is set to true, the failOnError of the Gradle RAT task is set to false.This causes the failure() condition in the Annotate Rat errors step of the workflow (build.yml) to evaluate as false, preventing the Annotate Rat errors step from being executed. Does this align with what we expect?

I see what you mean. In this case, let's do the same as we do for checkstyle and let RAT failures fail the build. Then, we will hit the failure() condition as expected. To do this, I think we just need to remove the ignoreRatFailure property.

… the build

This change aligns the behavior of the RAT task with the checkstyle task, ensuring that any RAT failures will trigger the `failure()` condition in the workflow. This should enable the `Annotate Rat errors step` to execute as expected.
@LoganZhuZzz
Copy link
Contributor Author

Now, it meets our expectations. Any suggestions? @mumrah

Output Results

eb639b877fc14ba94f172149b6e13ce edd5a162cc733152aad8930c8a35890

Link to Results

@mumrah
Copy link
Member

mumrah commented Sep 30, 2024

@LoganZhuZzz, looks great! Please revert the temporary files and I'll merge this.

@LoganZhuZzz
Copy link
Contributor Author

@LoganZhuZzz, looks great! Please revert the temporary files and I'll merge this.

Thanks for the guidance. @mumrah

Copy link
Member

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

LGTM, thanks @LoganZhuZzz!

edit: Also, thanks for the well written and well formatted PR :)

@mumrah mumrah merged commit 06d2649 into apache:trunk Oct 1, 2024
9 checks passed
tedyu pushed a commit to tedyu/kafka that referenced this pull request Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Gradle build or GitHub Actions ci-approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants