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

Truncate ecosystem comment when necessary #6899

Merged
merged 2 commits into from
Aug 28, 2023
Merged

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Aug 26, 2023

Summary

This PR ensures that the PR comment action always creates a comment, even if the ecosystem results are huge by:

  • Testing on the outcome of the step rather the comment text to prevent an out of memory error
  • Use another PR comment plugin that automatically truncates the text if it exceeds GitHubs limit of 65536 characters.

It would be nice if we could add a custom message when the results are truncated but the plugin does not support that. I think this is already a huge improvement because it prevents that we loose the ecosystem result when it's most needed

Test Plan

I commented out the traversal of the class body in checker to generate a large regression.

@MichaReiser
Copy link
Member Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@MichaReiser MichaReiser marked this pull request as draft August 26, 2023 14:42
@codspeed-hq
Copy link

codspeed-hq bot commented Aug 26, 2023

CodSpeed Performance Report

Merging #6899 will not alter performance

Comparing ecosystem-truncate-comment (4f8ddd1) with main (f33277a)

Summary

✅ 16 untouched benchmarks

@MichaReiser MichaReiser force-pushed the ecosystem-truncate-comment branch 11 times, most recently from 9580b93 to 911ced8 Compare August 27, 2023 08:51
@github-actions
Copy link
Contributor

github-actions bot commented Aug 27, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

@MichaReiser MichaReiser added the internal An internal refactor or improvement label Aug 27, 2023
@MichaReiser MichaReiser marked this pull request as ready for review August 27, 2023 09:12
comment_tag: PR Check Results
comment-d: ${{ steps.find-comment.outputs.comment-id }}
issue-number: ${{ steps.pr-number.outputs.pr-number }}
body-path: comment.txt
Copy link
Member Author

Choose a reason for hiding this comment

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

It needs to be a file to avoid OOMing when loading the content

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Thank you!

Is "comment << EOF" intended?

Screen Shot 2023-08-27 at 9 52 16 AM

@MichaReiser
Copy link
Member Author

Is "comment << EOF" intended?

Sure 😆

@github-actions
Copy link
Contributor

github-actions bot commented Aug 28, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

@github-actions
Copy link
Contributor

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

1 similar comment
@github-actions
Copy link
Contributor

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

@MichaReiser MichaReiser enabled auto-merge (squash) August 28, 2023 07:49
@MichaReiser MichaReiser merged commit 30ebf7f into main Aug 28, 2023
16 checks passed
@MichaReiser MichaReiser deleted the ecosystem-truncate-comment branch August 28, 2023 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants