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

[Merged by Bors] - feat: benches with criterion and dedicated workflow #2770

Conversation

EstebanBorai
Copy link
Contributor

@EstebanBorai EstebanBorai commented Nov 3, 2022

Replaces benchmarking with Criterion crate and provides a dedicated
workflow for running benchmarks as part of CI. As we add more benchmarks
to source code, the benchmarks workflow will grow with configuration for each
crate in the project.

@EstebanBorai
Copy link
Contributor Author

EstebanBorai commented Nov 3, 2022

@sehz , @morenol looks like we will have to provide special permissions for Benchmark workflow in order to allow commenting on commit:

Run benchmark-action/github-action-benchmark@v1
Error: Resource not accessible by integration

Actual Log


I believe this is related: https://stackoverflow.com/questions/70435286/resource-not-accessible-by-integration-on-github-post-repos-owner-repo-ac

@EstebanBorai EstebanBorai marked this pull request as ready for review November 3, 2022 02:36
@sehz
Copy link
Contributor

sehz commented Nov 3, 2022

CI failure

.github/workflows/benchmarks.yml Show resolved Hide resolved
.github/workflows/benchmarks.yml Outdated Show resolved Hide resolved
@EstebanBorai EstebanBorai force-pushed the eborai/feat/dedicated-workflow-for-benchmarks branch from 438b83c to 17d9bc0 Compare November 3, 2022 04:03
@EstebanBorai
Copy link
Contributor Author

CI failure

Yeah, its related to permissions I left this comment here: #2770 (comment).

I see other people complaining about similar issue here: actions/first-interaction#10

@EstebanBorai
Copy link
Contributor Author

Checking on the GitHub Token Permissions I can see we can only read PR:

GITHUB_TOKEN Permissions
  Metadata: read
  PullRequests: read

I think we could add an custom token for this workflow which allows write perhaps?

Actual Log

I also opened an issue on the action repository to ask for permissions scopes we could use for our workflow.

@morenol
Copy link
Contributor

morenol commented Nov 3, 2022

I guess that we could make it only run on master branch, I think that for pushes to that branch we won't get permission issues

Copy link
Contributor

@sehz sehz left a comment

Choose a reason for hiding this comment

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

maybe one way to solve permission issues is only cache the benchmark only master

.github/workflows/benchmarks.yml Outdated Show resolved Hide resolved
@morenol
Copy link
Contributor

morenol commented Nov 3, 2022

bors r+

bors bot pushed a commit that referenced this pull request Nov 3, 2022
Replaces benchmarking with Criterion crate and provides a dedicated
workflow for running benchmarks as part of CI. As we add more benchmarks
to source code, the benchmarks workflow will grow with configuration for each
crate in the project.
@morenol
Copy link
Contributor

morenol commented Nov 3, 2022

ohh, clippy issues!

@bors
Copy link

bors bot commented Nov 3, 2022

Build failed:

@EstebanBorai
Copy link
Contributor Author

@morenol reviewing!

@EstebanBorai
Copy link
Contributor Author

ohh, clippy issues!

Doesn't look related to this PR: https://github.com/infinyon/fluvio/actions/runs/3386673257/jobs/5626387694#step:8:1252
Will fix in a separate one, to also focus tests and checks on such change instead of introducing both things here.

@EstebanBorai
Copy link
Contributor Author

ohh, clippy issues!

Doesn't look related to this PR: https://github.com/infinyon/fluvio/actions/runs/3386673257/jobs/5626387694#step:8:1252 Will fix in a separate one, to also focus tests and checks on such change instead of introducing both things here.

Just saw you took care of this already here: #2771 Great!

@morenol
Copy link
Contributor

morenol commented Nov 3, 2022

bors retry

bors bot pushed a commit that referenced this pull request Nov 3, 2022
Replaces benchmarking with Criterion crate and provides a dedicated
workflow for running benchmarks as part of CI. As we add more benchmarks
to source code, the benchmarks workflow will grow with configuration for each
crate in the project.
@bors
Copy link

bors bot commented Nov 3, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat: benches with criterion and dedicated workflow [Merged by Bors] - feat: benches with criterion and dedicated workflow Nov 3, 2022
@bors bors bot closed this Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants