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

CI(clang-format): Post fixes as code suggestions if possible when running on a PR #1038

Merged
merged 2 commits into from
Mar 10, 2024

Conversation

echoix
Copy link
Member

@echoix echoix commented Mar 10, 2024

This is the continuation of #1035, and will be adapted back to OSGeo/grass#3284 as a final solution.

After a hundred iterations or so, I've got a solution that I'm satisfied with. This allows linter formatting in a PR from a forked pull request to do so without any special secret access, and also be able to add PR review comments, that need pull-request: write permissions. These permissions cannot be granted for PRs coming from forks. Since all the PRs come from forks, we could never use any write permissions from our forks. It also avoids using pull_request_target with a checkout of the external code, which is something that should never be done.

Instead, it runs another workflow triggered by the completion of another workflow, through the workflow_run trigger. That second workflow takes 3 seconds, max 10 including startup. It won't appear as a check for a PR, but still manages to use the event's payload information available to apply to the PR. The fixes that need to be added as code suggestion comments are fed through the upload of a diff artifact. It is then easily downloaded in the second workflow.

Note that PR review comments can only be added to lines in the diff context, that is 3 lines above or below the changed lines of a PR, just like the web interface: the API isn't different. The code suggestion comments are added on a best-effort basis to facilitate contributors to finish a PR. So only upgrading the clang-format version in a PR won't be able to post suggestions on all changes in the repo, since the affected lines aren't changed in that PR.

I designed this solution to be able to accept and post code suggestions for other tools in the near future. Other workflows could upload diffs from multiple tools in the same artifact, and only the tool name and the file name needs to be handled by conditional expressions (if: ...) or a bash loop (careful to not inject the file names from the artifact, as it is untrusted inputs; prefer whitelisting the expected file names to match to). For now, it is implemented for clang-format.

This time, I created a new organization, so I could develop with the forked-repo situation from the start. Once this PR is merged, the "callback" workflow that post code suggestions will be enabled, and will work right away.

@echoix echoix force-pushed the post-pr-review-suggestions branch from 9031f08 to 22729ad Compare March 10, 2024 17:18
@echoix echoix added CI Continuous integration C Related code is in C C++ Related code is in C++ labels Mar 10, 2024
@echoix echoix merged commit c73f0f0 into OSGeo:grass8 Mar 10, 2024
7 checks passed
@echoix echoix deleted the post-pr-review-suggestions branch March 10, 2024 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C Related code is in C C++ Related code is in C++ CI Continuous integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant