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

PR Benchmark Diffing #144

Merged
merged 2 commits into from
Jan 19, 2022
Merged

PR Benchmark Diffing #144

merged 2 commits into from
Jan 19, 2022

Conversation

MatthewCroughan
Copy link
Collaborator

@MatthewCroughan MatthewCroughan commented Jan 19, 2022

This adds a new effect via https://github.com/hercules-ci/hercules-ci-effects which takes a pull request and performs a diff on the benchmark by using nix run .#benchmark on the PR HEAD and on the merge-base of that PR.

@L-as
Copy link
Member

L-as commented Jan 19, 2022

Please retarget to staging...

@L-as L-as changed the base branch from master to staging January 19, 2022 22:22
@MatthewCroughan MatthewCroughan marked this pull request as draft January 19, 2022 22:23
@L-as
Copy link
Member

L-as commented Jan 19, 2022

Is there any issue on GH for the bug?

@MatthewCroughan
Copy link
Collaborator Author

@L-as No, I'm in the process of making one.

@MatthewCroughan
Copy link
Collaborator Author

Turns out there was no bug, it was just a missing ) in the effectScript which was easily conflated with the Nix stdenv when debugging.

Hercules CI was not configured optimally for this repo. I've renamed
nixCi to ciNix and configured it to be consistent with the Hercules CI
template, documentation and our other repos. It is no longer an
attribute that can be evaluated, instead Hercules CI is supposed to
simply build all of our existing flake attribute outputs, instead of
being a set that contains all of them recursively.
@MatthewCroughan MatthewCroughan force-pushed the mc/benchmark-diff branch 2 times, most recently from 9a7a2d2 to 6f059de Compare January 19, 2022 23:23
@MatthewCroughan MatthewCroughan requested a review from srid January 19, 2022 23:31
@MatthewCroughan MatthewCroughan marked this pull request as ready for review January 19, 2022 23:31
Copy link
Member

@srid srid left a comment

Choose a reason for hiding this comment

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

Except for that one comment, we are good I think.

flake.nix Outdated Show resolved Hide resolved
This adds a new Hercules CI effect which takes a pull request and
performs a diff on the benchmark by using `nix run .#benchmark` on the
HEAD of the PR, and on the merge-base of that PR, which effectively
gives us a performance measurement before the PR and after the PR.
@srid srid mentioned this pull request Jan 19, 2022
@MatthewCroughan MatthewCroughan linked an issue Jan 19, 2022 that may be closed by this pull request
@srid srid merged commit 1920514 into staging Jan 19, 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.

GitHub PR: benchmark diff
3 participants