search

ZedThree / clang-tidy-review

Create a pull request review based on clang-tidy warnings
MIT License
90 stars 45 forks source link

Error posting comments about code from forks #49

Closed pajlada closed 2 years ago

pajlada commented 2 years ago

When an outside contributor makes a PR, clang-tidy can review the code but it can't post comments due to a lack of permissions.

The way GitHub Actions is set up is that forks have very limited permissions, including the inability to access any repository or organization secrets you've set up with extra permissions.

I see two ways of solving this: 1) Making the action running this unsafe by using pull_request_target - this would mean that the person creating the fork & PR have access to your secrets with their modified code, which means they could easily abuse or leak the secrets so this is not viable for public repositories. 2) Splitting the action up into two parts. The unprivileged action that runs clang-tidy, parses errors/warnings etc and adds them to ::set-output, and the privileged action that parses the output set from the first action and posts the actual PR comments. (Similar to the example posted here)

I would be interested in implementing 2), but I'd like to hear your thoughts about before I do any development in case I've missed something obvious or if you feel some part of the solution wouldn't fit into this action.

Example logs from the error occurring: (click to expand) ``` 2022-07-31T20:20:45.0448070Z ##[group]Run ZedThree/clang-tidy-review@v0.8.4 2022-07-31T20:20:45.0448519Z with: 2022-07-31T20:20:45.0448686Z build_dir: build 2022-07-31T20:20:45.0448889Z config_file: .clang-tidy 2022-07-31T20:20:45.0449243Z token: *** 2022-07-31T20:20:45.0449496Z base_dir: /home/runner/work/chatterino2/chatterino2 2022-07-31T20:20:45.0449740Z clang_tidy_version: 12 2022-07-31T20:20:45.0450087Z clang_tidy_checks: -*,performance-*,readability-*,bugprone-*,clang-analyzer-*,cppcoreguidelines-*,mpi-*,misc-* 2022-07-31T20:20:45.0450427Z include: *.[ch],*.[ch]xx,*.[ch]pp,*.[ch]++,*.cc,*.hh 2022-07-31T20:20:45.0450655Z max_comments: 25 2022-07-31T20:20:45.0450841Z pr: 3886 2022-07-31T20:20:45.0451037Z repo: Chatterino/chatterino2 2022-07-31T20:20:45.0451258Z env: 2022-07-31T20:20:45.0451490Z pythonLocation: /opt/hostedtoolcache/Python/3.10.5/x64 2022-07-31T20:20:45.0451906Z LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.10.5/x64/lib:/home/runner/work/chatterino2/chatterino2/qt/Qt/5.15.2/gcc_64/lib 2022-07-31T20:20:45.0452312Z PKG_CONFIG_PATH: /home/runner/work/chatterino2/chatterino2/qt/Qt/5.15.2/gcc_64/lib/pkgconfig 2022-07-31T20:20:45.0452977Z Qt5_Dir: /home/runner/work/chatterino2/chatterino2/qt/Qt/5.15.2/gcc_64 2022-07-31T20:20:45.0453429Z Qt5_DIR: /home/runner/work/chatterino2/chatterino2/qt/Qt/5.15.2/gcc_64 2022-07-31T20:20:45.0453736Z QT_PLUGIN_PATH: /home/runner/work/chatterino2/chatterino2/qt/Qt/5.15.2/gcc_64/plugins 2022-07-31T20:20:45.0454058Z QML2_IMPORT_PATH: /home/runner/work/chatterino2/chatterino2/qt/Qt/5.15.2/gcc_64/qml 2022-07-31T20:20:45.0454303Z ##[endgroup] 2022-07-31T20:20:45.0834749Z ##[command]/usr/bin/docker run --name cd98f5e81d1ff62384aabaebb014676eef0ab_833c71 --label 4cd98f --workdir /github/workspace --rm -e pythonLocation -e LD_LIBRARY_PATH -e PKG_CONFIG_PATH -e Qt5_Dir -e Qt5_DIR -e QT_PLUGIN_PATH -e QML2_IMPORT_PATH -e INPUT_BUILD_DIR -e INPUT_CONFIG_FILE -e INPUT_TOKEN -e INPUT_BASE_DIR -e INPUT_CLANG_TIDY_VERSION -e INPUT_CLANG_TIDY_CHECKS -e INPUT_INCLUDE -e INPUT_EXCLUDE -e INPUT_APT_PACKAGES -e INPUT_CMAKE_COMMAND -e INPUT_MAX_COMMENTS -e INPUT_PR -e INPUT_REPO -e HOME -e GITHUB_JOB -e GITHUB_REF -e GITHUB_SHA -e GITHUB_REPOSITORY -e GITHUB_REPOSITORY_OWNER -e GITHUB_RUN_ID -e GITHUB_RUN_NUMBER -e GITHUB_RETENTION_DAYS -e GITHUB_RUN_ATTEMPT -e GITHUB_ACTOR -e GITHUB_TRIGGERING_ACTOR -e GITHUB_WORKFLOW -e GITHUB_HEAD_REF -e GITHUB_BASE_REF -e GITHUB_EVENT_NAME -e GITHUB_SERVER_URL -e GITHUB_API_URL -e GITHUB_GRAPHQL_URL -e GITHUB_REF_NAME -e GITHUB_REF_PROTECTED -e GITHUB_REF_TYPE -e GITHUB_WORKSPACE -e GITHUB_ACTION -e GITHUB_EVENT_PATH -e GITHUB_ACTION_REPOSITORY -e GITHUB_ACTION_REF -e GITHUB_PATH -e GITHUB_ENV -e GITHUB_STEP_SUMMARY -e RUNNER_OS -e RUNNER_ARCH -e RUNNER_NAME -e RUNNER_TOOL_CACHE -e RUNNER_TEMP -e RUNNER_WORKSPACE -e ACTIONS_RUNTIME_URL -e ACTIONS_RUNTIME_TOKEN -e ACTIONS_CACHE_URL -e GITHUB_ACTIONS=true -e CI=true -v "/var/run/docker.sock":"/var/run/docker.sock" -v "/home/runner/work/_temp/_github_home":"/github/home" -v "/home/runner/work/_temp/_github_workflow":"/github/workflow" -v "/home/runner/work/_temp/_runner_file_commands":"/github/file_commands" -v "/home/runner/work/chatterino2/chatterino2":"/github/workspace" 4cd98f:5e81d1ff62384aabaebb014676eef0ab "--clang_tidy_binary=clang-tidy-12" "--token=***" "--repo=Chatterino/chatterino2" "--pr=3886" "--build_dir=build" "--base_dir=/home/runner/work/chatterino2/chatterino2" "--clang_tidy_checks=-*,performance-*,readability-*,bugprone-*,clang-analyzer-*,cppcoreguidelines-*,mpi-*,misc-*" "--config_file=.clang-tidy" "--include='*.[ch],*.[ch]xx,*.[ch]pp,*.[ch]++,*.cc,*.hh'" "--exclude=''" "--apt-packages=" "--cmake-command=''" 2022-07-31T20:20:45.8627385Z Found 'build/compile_commands.json', updating absolute paths 2022-07-31T20:20:45.8628286Z Replacing '/home/runner/work/chatterino2/chatterino2' with '/github/workspace' 2022-07-31T20:20:46.7076671Z 2022-07-31T20:20:46.7077060Z Diff from GitHub PR: 2022-07-31T20:20:46.7077901Z [, , ] 2022-07-31T20:20:46.7078240Z 2022-07-31T20:20:46.7078356Z include: *.[ch], file list now: [] 2022-07-31T20:20:46.7078622Z include: *.[ch]xx, file list now: [] 2022-07-31T20:20:46.7079484Z include: *.[ch]pp, file list now: ['src/singletons/helper/GifTimer.cpp', 'src/singletons/helper/GifTimer.hpp'] 2022-07-31T20:20:46.7080044Z include: *.[ch]++, file list now: ['src/singletons/helper/GifTimer.cpp', 'src/singletons/helper/GifTimer.hpp'] 2022-07-31T20:20:46.7080772Z include: *.cc, file list now: ['src/singletons/helper/GifTimer.cpp', 'src/singletons/helper/GifTimer.hpp'] 2022-07-31T20:20:46.7081316Z include: *.hh, file list now: ['src/singletons/helper/GifTimer.cpp', 'src/singletons/helper/GifTimer.hpp'] 2022-07-31T20:20:46.7081847Z exclude: , file list now: ['src/singletons/helper/GifTimer.cpp', 'src/singletons/helper/GifTimer.hpp'] 2022-07-31T20:20:46.7082563Z Checking these files: ['src/singletons/helper/GifTimer.cpp', 'src/singletons/helper/GifTimer.hpp'] 2022-07-31T20:20:46.7087788Z Line filter for clang-tidy: 2022-07-31T20:20:46.7088325Z ["{'name': 'src/singletons/helper/GifTimer.cpp', 'lines': [[11, 11]]}","{'name': 'src/singletons/helper/GifTimer.hpp', 'lines': [[8, 8]]}"] 2022-07-31T20:20:46.7088786Z 2022-07-31T20:20:46.7088953Z Using config: -config-file=.clang-tidy 2022-07-31T20:20:46.7089646Z ##[group]Running: 2022-07-31T20:20:46.7090479Z clang-tidy-12 -p=build -config-file=.clang-tidy -line-filter=["{'name': 'src/singletons/helper/GifTimer.cpp', 'lines': [[11, 11]]}","{'name': 'src/singletons/helper/GifTimer.hpp', 'lines': [[8, 8]]}"] src/singletons/helper/GifTimer.cpp src/singletons/helper/GifTimer.hpp --export-fixes=clang_tidy_review.yaml 2022-07-31T20:21:16.7796055Z ##[endgroup] 2022-07-31T20:21:16.7843853Z 2022-07-31T20:21:16.7843860Z 2022-07-31T20:21:16.7844513Z clang-tidy failed with return code 1 and error: 2022-07-31T20:21:16.7844819Z 108928 warnings and 1 error generated. 2022-07-31T20:21:16.7845185Z Error while processing /github/workspace/src/singletons/helper/GifTimer.cpp. 2022-07-31T20:21:16.7845521Z 194107 warnings and 1 error generated. 2022-07-31T20:21:16.7845857Z Error while processing /github/workspace/src/singletons/helper/GifTimer.hpp. 2022-07-31T20:21:16.7846334Z Suppressed 194106 warnings (194099 in non-user code, 7 due to line filter). 2022-07-31T20:21:16.7846935Z Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well. 2022-07-31T20:21:16.7847820Z Found compiler error(s). 2022-07-31T20:21:16.7847960Z 2022-07-31T20:21:16.7848040Z Output was: 2022-07-31T20:21:16.7848514Z /github/workspace/src/common/SignalVector.hpp:5:10: error: 'boost/noncopyable.hpp' file not found [clang-diagnostic-error] 2022-07-31T20:21:16.7849075Z #include 2022-07-31T20:21:16.7849290Z ^ 2022-07-31T20:21:16.7849865Z /github/workspace/src/singletons/helper/GifTimer.hpp:8:25: warning: invalid case style for global variable 'gifFrameLength' [readability-identifier-naming] 2022-07-31T20:21:16.7850313Z constexpr long unsigned gifFrameLength = 20; 2022-07-31T20:21:16.7850574Z ^~~~~~~~~~~~~~ 2022-07-31T20:21:16.7850806Z GIF_FRAME_LENGTH 2022-07-31T20:21:16.7850956Z 2022-07-31T20:21:16.7851042Z Took: 0:00:30.071182 2022-07-31T20:21:16.7851339Z clang-tidy had the following warnings: 2022-07-31T20:21:16.7853910Z {'MainSourceFile': '/github/workspace/src/singletons/helper/GifTimer.cpp', 'Diagnostics': [{'DiagnosticName': 'clang-diagnostic-error', 'DiagnosticMessage': {'Message': "'boost/noncopyable.hpp' file not found", 'FilePath': '/github/workspace/src/common/SignalVector.hpp', 'FileOffset': 71, 'Replacements': []}, 'Level': 'Error', 'BuildDirectory': '/github/workspace/build/src', 'Ranges': [{'FilePath': '/github/workspace/src/common/SignalVector.hpp', 'FileOffset': 71, 'Length': 23}]}, {'DiagnosticName': 'readability-identifier-naming', 'DiagnosticMessage': {'Message': "invalid case style for global variable 'gifFrameLength'", 'FilePath': '/github/workspace/src/singletons/helper/GifTimer.hpp', 'FileOffset': 119, 'Replacements': [{'FilePath': '/github/workspace/src/singletons/helper/GifTimer.hpp', 'Offset': 119, 'Length': 14, 'ReplacementText': 'GIF_FRAME_LENGTH'}]}, 'Level': 'Warning', 'BuildDirectory': '/github/workspace/build/src'}]} 2022-07-31T20:21:16.7860688Z ##[group]Creating review from warnings 2022-07-31T20:21:16.7861297Z {'Message': "'boost/noncopyable.hpp' file not found", 'FilePath': '/github/workspace/src/common/SignalVector.hpp', 'FileOffset': 71, 'Replacements': []} 2022-07-31T20:21:16.7861820Z line_num=4; line_offset=9; source_line='#include ' 2022-07-31T20:21:16.7862061Z 2022-07-31T20:21:16.7862465Z WARNING: Skipping comment for file 'src/common/SignalVector.hpp' not in PR changeset. Comment body is: 2022-07-31T20:21:16.7862933Z warning: 'boost/noncopyable.hpp' file not found [clang-diagnostic-error] 2022-07-31T20:21:16.7863210Z ```cpp 2022-07-31T20:21:16.7863423Z #include 2022-07-31T20:21:16.7863635Z ^ 2022-07-31T20:21:16.7863793Z ``` 2022-07-31T20:21:16.7863897Z 2022-07-31T20:21:16.7865036Z {'Message': "invalid case style for global variable 'gifFrameLength'", 'FilePath': '/github/workspace/src/singletons/helper/GifTimer.hpp', 'FileOffset': 119, 'Replacements': [{'FilePath': '/github/workspace/src/singletons/helper/GifTimer.hpp', 'Offset': 119, 'Length': 14, 'ReplacementText': 'GIF_FRAME_LENGTH'}]} 2022-07-31T20:21:16.7866177Z line_num=7; line_offset=24; source_line='constexpr long unsigned gifFrameLength = 20;' 2022-07-31T20:21:16.7866498Z 2022-07-31T20:21:16.7866743Z ---------- 2022-07-31T20:21:16.7867117Z old_line='constexpr long unsigned gifFrameLength = 20;' 2022-07-31T20:21:16.7867558Z new_line='constexpr long unsigned GIF_FRAME_LENGTH = 20;' 2022-07-31T20:21:16.7867865Z ---------- 2022-07-31T20:21:16.7868800Z ##[endgroup] 2022-07-31T20:21:16.7869013Z Created the following review: 2022-07-31T20:21:16.7869330Z {'body': 'clang-tidy made some suggestions', 2022-07-31T20:21:16.7869809Z 'comments': [{'body': "warning: invalid case style for global variable 'gifFrameLength' [readability-identifier-naming]\n" 2022-07-31T20:21:16.7870186Z '\n' 2022-07-31T20:21:16.7870449Z '```suggestion\n' 2022-07-31T20:21:16.7870792Z 'constexpr long unsigned GIF_FRAME_LENGTH = 20;\n' 2022-07-31T20:21:16.7871091Z '```\n', 2022-07-31T20:21:16.7871329Z 'line': 8, 2022-07-31T20:21:16.7871632Z 'path': 'src/singletons/helper/GifTimer.hpp', 2022-07-31T20:21:16.7871933Z 'side': 'RIGHT'}], 2022-07-31T20:21:16.7872178Z 'event': 'COMMENT'} 2022-07-31T20:21:16.7872410Z Removing already posted or extra comments 2022-07-31T20:21:16.9058447Z Posting the review: 2022-07-31T20:21:16.9058849Z {'body': 'clang-tidy made some suggestions', 2022-07-31T20:21:16.9059292Z 'comments': [{'body': 'warning: invalid case style for global variable ' 2022-07-31T20:21:16.9059930Z "'gifFrameLength' [readability-identifier-naming]\n" 2022-07-31T20:21:16.9060279Z '\n' 2022-07-31T20:21:16.9060567Z '```suggestion\n' 2022-07-31T20:21:16.9060957Z 'constexpr long unsigned GIF_FRAME_LENGTH = 20;\n' 2022-07-31T20:21:16.9061268Z '```\n', 2022-07-31T20:21:16.9061713Z 'line': 8, 2022-07-31T20:21:16.9062075Z 'path': 'src/singletons/helper/GifTimer.hpp', 2022-07-31T20:21:16.9062413Z 'side': 'RIGHT'}], 2022-07-31T20:21:16.9062688Z 'event': 'COMMENT'} 2022-07-31T20:21:17.1406031Z {"message":"Resource not accessible by integration","documentation_url":"https://docs.github.com/rest/reference/pulls#create-a-review-for-a-pull-request"} 2022-07-31T20:21:17.1414461Z Traceback (most recent call last): 2022-07-31T20:21:17.1414984Z File "/review.py", line 810, in 2022-07-31T20:21:17.1415247Z main( 2022-07-31T20:21:17.1415468Z File "/review.py", line 676, in main 2022-07-31T20:21:17.1415728Z pull_request.post_review(trimmed_review) 2022-07-31T20:21:17.1416020Z File "/review.py", line 115, in post_review 2022-07-31T20:21:17.1416308Z post_review_response.raise_for_status() 2022-07-31T20:21:17.1417027Z File "/usr/local/lib/python3.8/dist-packages/requests/models.py", line 1021, in raise_for_status 2022-07-31T20:21:17.1417401Z raise HTTPError(http_error_msg, response=self) 2022-07-31T20:21:17.1417868Z requests.exceptions.HTTPError: 403 Client Error: Forbidden for url: https://api.github.com/repos/Chatterino/chatterino2/pulls/3886/reviews ```
ZedThree commented 2 years ago

Hi @pajlada, getting clang-tidy-review working nicely with forks would be really great!

Is perhaps a third way to use annotations instead of comments? It looks like that would be limited to only 10 annotations, which is perhaps a bit small. I'm also not sure if they handle suggestions, which is one of the really nice things about using comments. I'm actually not even sure if they do work from forks.

Would your suggestion mean people would need to start using two separate actions (and we would need two repos) in order to use clang-tidy-review? I'm not opposed to that, but I worry it would make it a little bit harder to use (and also to develop and test!). Do you know if it would be possible to define both actions in the same repo? That would be a little bit nicer at least. I guess submodules might always work.

I wonder if there would be a way to make it work so that if run from a fork, and the user was not using the split action, then it would post a failure as an annotation with instructions on how to use the split action?

pajlada commented 2 years ago

Annotations definitely looks like a suitable alternative, however I believe the 10 annotation limit is correct, and losing the Suggestions part is unfortunate.

For 2), I would also prefer using a single repo & action since there would need to be some code shared (e.g. posting the actual suggestions). I think this can done by having two inputs the user can set. One that tell the action to save the suggestions as an output or artifact instead of actually posting them, and one that fetches that output or artifact and then applies the suggestions.

# Action 1
Name: Review
...
- uses: ZedThree/clang-tidy-review@v0.8.4
  id: review
  with:
    output_suggestions: true
...
# Action 2
Name: Post suggestions

on:
  workflow_run:
    workflows: ["Receive PR"]
    types:
      - completed
...
- uses: ZedThree/clang-tidy-review@v0.8.4
  id: review
  with:
    post_suggestions: true
...

The default behaviour of the action would stay the same, but the output_suggestions and post_suggestions can be used together to split the workload up (naming would need more thought of course). In practice it might need to be slightly more complicated (might need to output the information as an artifact rather than using ::set-output), but it should stay fairly simple

I wonder if there would be a way to make it work so that if run from a fork, and the user was not using the split action, then it would post a failure as an annotation with instructions on how to use the split action?

Handling the error gracefully with instructions sounds like a great approach

ZedThree commented 2 years ago

While that sort of split using switches to control output vs posting would work, my main worry is the time and resource use. This is a docker action, and it seems Github does no caching of the images so it rebuilds it from scratch each time which takes a couple of minutes. The post action looks like it should be pretty cheap to run as it can be done in javascript (or using the actions/github-script action).

It actually does look like it's possible to have multiple actions in the same repo: https://github.com/github/codeql-action

So we'd have something like:

clang-tidy-review
├── review
│   └── action.yml
└── post_comments
    └── action.yml

And users would have:

# Action 1
Name: clang-tidy-review
...
- uses: ZedThree/clang-tidy-review/review@v0.8.4
  id: review
  with:
    post_comments: false
...
# Action 2
Name: Post suggestions

on:
  workflow_run:
    workflows: ["clang-tidy-review"]
    types:
      - completed
...
- uses: ZedThree/clang-tidy-review/post_comments@v0.8.4
  id: review
...
pajlada commented 2 years ago

I have been experimenting with the GitHub action part of this, and it can be achieved as a single repo as you said.

Standard operation

# Action 1 standard operation
...
  - uses: ZedThree/clang-tidy-review@v0.8.4
...

Split operation

# Action 1 split operation
...
  - uses: ZedThree/clang-tidy-review@v0.8.4
    with:
      post_comments: false
  - uses: actions/upload-artifact@v3
    with:
      name: clang-tidy-review
      path: clang_tidy_review.yaml

# Action 2 split operation
...
  - name: 'Download artifact'
    uses: actions/github-script@v6
    with:
      script: |
        const artifacts = await github.rest.actions.listWorkflowRunArtifacts({
           owner: context.repo.owner,
           repo: context.repo.repo,
           run_id: ${{github.event.workflow_run.id }},
        });
        const matchArtifact = artifacts.data.artifacts.filter((artifact) => {
          return artifact.name == "clang-tidy-review"
        })[0];
        const download = await github.rest.actions.downloadArtifact({
           owner: context.repo.owner,
           repo: context.repo.repo,
           artifact_id: matchArtifact.id,
           archive_format: 'zip',
        });
        const fs = require('fs');
        fs.writeFileSync('${{github.workspace}}/clang-tidy-review.zip', Buffer.from(download.data));
  - name: 'Unzip artifact'
    run: unzip clang-tidy-review.zip

  - uses: ZedThree/clang-tidy-review/post_comments@v0.8.4

My main question for you @ZedThree before I start working on a PR is: Do you want to keep the review action in the root of the repository, or do you want it moved to a review subdirectory? I personally prefer the former to be as little of a burden as possible (users won't have to change their workflow at all when they update to the version that has this feature), and I haven't found any downsides to as the full repo is cloned regardless when the action is built.

And as an additional note on the size of the split operation; I've found that the PyGithub library used is kind of out of date with the GitHub API and doesn't have bindings for any of the methods we'd want to access (Upload Artifact, List Workflow Artifacts, and Download Artifact), which means that the implementation will have to be a bit more ugly and involve some additional actions.

FlorianReimold commented 2 years ago

We have adopted clang-tidy-review in our eCAL Project a while back. Since we moved to Eclipse and now use it for Eclipse eCAL, we have adopted a fork-based workflow. And - as one can imagine - clang-tidy-review doesn't work great with that new workflow.

Therefore I would be happy about any workaround. For me, it would also be acceptable to have the clang-tidy-review-action only post a nicely formatted comment, without leaving an actual github review.

@pajlada : Thanks for experimenting and taking care of this. If you need any assistance, please let me know.

pajlada commented 2 years ago

@FlorianReimold I've got it minimally working in a fork environment. Will make a PR this weekend for feedback to which I'll request you as a reviewer to ensure we've captured everyone's workflow needs!