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

[#1488]: fix(CI): make jacoco report work properly #2747

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

yijhenlin
Copy link
Contributor

@yijhenlin yijhenlin commented Apr 1, 2024

What changes were proposed in this pull request?

  1. Skip tests in integration-test-common if -PskipITs is passed
  2. Use proper jacoco report paths
    3. Add pull-requests write permissions in workflow (not work for forked repo)
  3. Output the report to workflow summary

Why are the changes needed?

  1. Refer to [Bug report] Code coverage tool JacocoReport does not work  #1488 (comment)
  2. The reports didn't include the submodules, like catalogs/catalog-jdbc-postgresql, clients/client-java, etc.
  3. If the write permission is not enable, it will get error msg Error: HttpError: Resource not accessible by integration

Fix: #1488

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

N/A

@yijhenlin
Copy link
Contributor Author

It seems like the permission in this repo cannot be effect by workflow setting.
will figure it out later.
image

@yijhenlin yijhenlin marked this pull request as draft April 1, 2024 03:45
@yijhenlin
Copy link
Contributor Author

yijhenlin commented Apr 1, 2024

Refer to https://docs.github.com/en/actions/security-guides/automatic-token-authentication#modifying-the-permissions-for-the-github_token

Note: Organization owners can prevent you from granting write access to the GITHUB_TOKEN at the repository level.

image

And here is the log of the action history, the permissions have changed from write to read.
image
image

@yijhenlin
Copy link
Contributor Author

Hi @yuqi1129 , I'm thinking that this issue may not be resolved in repository level.
One of the root cause is the permissions problem, please refer to the previous comment.

@yijhenlin
Copy link
Contributor Author

Update: It's the restriction on pull requests.

Workflow runs triggered by Dependabot pull requests run as if they are from a forked repository, and therefore use a read-only GITHUB_TOKEN.

Solutions from docs:

  1. Use pull_request_target make workflow run base on target repository, which means GITHUB_TOKEN have write permission. But the workflow won't apply the commit change from forked repository.
  2. Granting additional permissions
@jerryshao
Copy link
Contributor

@yijhenlin Would you please guide as on how to do this? I think it worked before, not sure which change makes it fail to work.

@yijhenlin
Copy link
Contributor Author

@jerryshao
If the repository is private, it has policies to allow pull requests using a GITHUB_TOKEN with write permission.
When the repository becomes public, these policies cannot be used due to security reasons.

Currently, the recommended way is through Github App to generate an authentication token with fine-grained permission and use this token for public repository.

@jerryshao
Copy link
Contributor

Thanks for the explanation, let me check this.

@yijhenlin
Copy link
Contributor Author

Hi @jerryshao
I've tried for a while and realize that it's not easy to pass secrets/tokens to a forked repo. (pull_request_target works, but it is too risky compared to the purpose)
So here is a workaround, output the report to workflow summary instead of comment.
WDYT?
image

@yijhenlin yijhenlin marked this pull request as ready for review April 11, 2024 05:28
- name: Jacoco Report to PR
id: jacoco
uses: madrapps/jacoco-report@v1.6.1
uses: yijhenlin/jacoco-report@summary_mode
Copy link
Contributor

Choose a reason for hiding this comment

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

So you build your own github action?

Copy link
Member

Choose a reason for hiding this comment

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

hi @yijhenlin Thank you for your contribution.
You fixed jacoco issue in your github repo? yijhenlin/jacoco-report@4305a5a

Copy link
Contributor Author

@yijhenlin yijhenlin Apr 12, 2024

Choose a reason for hiding this comment

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

Yes, I fork from madrapps/jacoco-report and patch an option to check if this idea works or not, since it didn't expose the report's raw data to user level.
I'll try to contribute back to madrapps/jacoco-report if the workaround is acceptable and you can fork your own too.

@jerryshao
Copy link
Contributor

@xunliu what's your opinion on it?

@xunliu
Copy link
Member

xunliu commented Apr 12, 2024

@xunliu what's your opinion on it?

I'm fork yijhenlin/jacoco-report@4305a5a to Datastrato org?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants