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

Add benchmark CI (not from fork) #2369

Closed
wants to merge 10 commits into from
Closed

Add benchmark CI (not from fork) #2369

wants to merge 10 commits into from

Conversation

kitlangton
Copy link
Member

@kitlangton kitlangton commented Aug 4, 2023

So, I already have this PR open here (#2368). But it would appear that PRs opened from forks are given a read-only GITHUB_TOKEN—which makes sense when you realize that if it didn't, anyone could simply open a PR and then have full read/write permissions over all pull requests. So, I'm opening this PR from a separate branch within this repo, to make sure I do in fact have the ability to create a comment on this PR from within my custom action.

UPDATE: Yup, it was successfully able to add the comment here. (#2369 (comment)) I'll do some research and see if I can come up with any workarounds or alternatives. It sure would be nice if there was a special "add comments to Pull Requests" permission, as that doesn't seem like such a crazy thing to do.

UPDATE AGAIN: Aha, so it appears the pull_request_target event is the solution. docs

UPDATE 3: Ha. Turns out that it's not a good idea to use pull_request_target when you do any sort of building: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/. The true recommended workaround would be to use two different workflows: one for checking out the PR code, running the benchmarks, and uploading the result as an artifact, and a second for downloading that artifact and commenting on the PR. A simpler option would be to post a markdown job summary, which will work with the normal pull_request trigger and will not require multiple workflows.

UPDATE 4: A job summary would look like this:

CleanShot 2023-08-05 at 09 23 37@2x

Here's an example from the zio-http workflow for a PR from a fork (you don't need special permissions to add a job summary, thankfully).

@github-actions github-actions bot added the maintenance Chore or Maintenance tasks label Aug 4, 2023
@mergify
Copy link

mergify bot commented Aug 4, 2023

⚠️ The sha of the head commit of this PR conflicts with #2368. Mergify cannot evaluate rules on this PR. ⚠️

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2023

🤖 Benchmark Comparison for Empty commit

Benchmark Previous Current Change
zio.benchmarks.HttpCombineEval.ok 2,227,802.97 ops/s 2,171,801.80 ops/s -2.51%

@zio zio deleted a comment from github-actions bot Aug 4, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2023

🤖 Benchmark Comparison for Empty commit

Benchmark Previous Current Change
zio.benchmarks.HttpCombineEval.ok 1,931,876.85 ops/s 2,229,123.93 ops/s 15.39%

@codecov-commenter
Copy link

Codecov Report

Patch and project coverage have no change.

Comparison is base (538761f) 63.21% compared to head (72c9570) 63.21%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2369   +/-   ##
=======================================
  Coverage   63.21%   63.21%           
=======================================
  Files         135      135           
  Lines        7032     7032           
  Branches     1185     1185           
=======================================
  Hits         4445     4445           
  Misses       2587     2587           

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Chore or Maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants