Skip to content

conditionally create comment #91

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

Merged
merged 9 commits into from
Mar 26, 2024
Merged

conditionally create comment #91

merged 9 commits into from
Mar 26, 2024

Conversation

2bndy5
Copy link
Contributor

@2bndy5 2bndy5 commented Mar 15, 2024

This better satisfies the maximum comment length imposed by GitHub while leaving the step summary length unhindered.

An added benefit here is that we only create a comment when it is needed. Previously, we were creating a comment to also get a tally of checks failed.

resolves #89

@2bndy5 2bndy5 added the bug Something isn't working label Mar 15, 2024
@2bndy5 2bndy5 force-pushed the conditionally-create-comment branch from c051ac8 to 563e69d Compare March 15, 2024 13:11
Copy link

codecov bot commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (07f63c2) to head (3d923dc).

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #91   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           20        21    +1     
  Lines         1436      1502   +66     
=========================================
+ Hits          1436      1502   +66     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@2bndy5
Copy link
Contributor Author

2bndy5 commented Mar 15, 2024

This approach should actually expedite CI runs that don't use thread-comments or step-summary features (with no breaking behavior).

I also prioritized feedback features that don't require special permissions. Meaning, if the CI run fails due to inadequate permissions, the step-summary and file-annotations will be done before cpp-linter raises an exception about required permission. Bare in mind that users can still use the outputs in case permission is not granted.

    steps:
      - uses: actions/checkout@v4
      - uses: cpp-linter/cpp-linter-action@v2
        id: linter
        # allow use of outputs in case of permission errors
        continue-on-error: true
        with:
          # no GITHUB_TOKEN in env means no permission for comments
          thread-comments: ${{ github.event_name == 'pull_request' && 'update' }}
      - name: Fail fast?!
        if: steps.linter.outputs.checks-failed > 0
        run: exit 1

2bndy5 added 8 commits March 26, 2024 00:20
This better satisfies the maximum comment length imposed by GitHub while leaving the step summary length unhindered.

An added benefit here is that we only create a comment when it is needed. Previously, we were creating a comment to also get a tally of checks failed.
also prefer to use kwargs for function calls to avoid mismatching values
@2bndy5 2bndy5 force-pushed the conditionally-create-comment branch 2 times, most recently from 8fc4d2b to fc94590 Compare March 26, 2024 21:30
@2bndy5
Copy link
Contributor Author

2bndy5 commented Mar 26, 2024

I'm confident enough that this is good to merge now. I added a test to make sure (& even manually inspected the comment output as markdown).

tests length limit for thread-comment vs step-summary

save test artifacts to temp path (for manual inspection)
@2bndy5 2bndy5 force-pushed the conditionally-create-comment branch from fc94590 to 3d923dc Compare March 26, 2024 21:45
@2bndy5 2bndy5 merged commit 914526c into main Mar 26, 2024
@2bndy5 2bndy5 deleted the conditionally-create-comment branch March 26, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Github API post comment error: body is too long
1 participant