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

Coverage comment experiment #3150

Closed

Conversation

alecgrieser
Copy link
Collaborator

I noticed that all of the comments are being sent to #3147, the change which introduced pull_request_comments.yml, which I think is a mistake. To avoid that, this tries to use a job within the PR

I noticed that all of the comments are being sent to FoundationDB#3147, the change which introduced `pull_request_comments.yml`, which I think is a mistake. To avoid that,
this tries to use a job within the PR
permissions:
checks: write
contents: read
pull-requests: write
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that this will work: pull_request workflows have their permissions always limited, since they are running code from the PR itself. (There is pull_request_target, but the protection is intentional.)

If the pull_request triggering the workflow_run isn't working properly, then we could write the PR number into an artifact and read from there in the workflow_run workflow, passing the number to the comment action. See https://github.com/Madrapps/jacoco-report/blob/main/src/action.ts#L92-L101 for how it tries to figure it out -- which is evidently not working -- and the available override.

@alecgrieser
Copy link
Collaborator Author

This ended up not working out, for reasons @MMcM alludes to. Thanks to #3158, we're now threading through the PR number a different way, which gets us what we want.

@alecgrieser alecgrieser deleted the associate-pr-number branch February 14, 2025 10:38
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.

2 participants