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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document plutarch-benchmark library & diffing workflow #167

Merged
merged 1 commit into from Jan 24, 2022

Conversation

emiflake
Copy link
Collaborator

This also fixes the lack of README.md under plutarch-benchmark which would cause issues for projects depending on plutarch with haskell.nix 馃Х

@emiflake emiflake changed the base branch from master to staging January 22, 2022 19:49
@emiflake
Copy link
Collaborator Author

Ah looks like this doesn't cause any of the effects to run on the PR, which is the workflow I think we're wanting to facilitate (i.e. running them before they get merged)
@MatthewCroughan is there a reason we can't ease the runIf restriction? Maybe run if staging is strictly a subset of HEAD or something the like.

@MatthewCroughan
Copy link
Collaborator

MatthewCroughan commented Jan 22, 2022

@emiflake Because Hercules/Nix doesn't give us any information about the merge-base. Which means we have to for now assume it's staging until Hercules-CI 0.9 comes out. Hercules also does not currently allow running CI against forked branches hercules-ci/support#10, only those that are part of the repo. That makes sense, because why would I want to allow you to run arbitrary code on the CI machine? If it allowed you to do it, it means any anonymous user on Github can also run arbitrary code on the CI machine, not just members of this organization. It's a simple security measure.

@emiflake
Copy link
Collaborator Author

If it allowed you to do it, it means any anonymous user on Github can also run arbitrary code on the CI machine, not just members of this organization. It's a simple security measure.

Yeah this is not really what I'm asking about in the comment above. I'm okay with that limitation already, I'm more so asking as to why the effect doesn't run on a PR (or on all non-fork commits, that would be fine too).

In the future, once hercules adds the button that starts it on fork PRs, then those with that access can execute it to see if there were no major regressions in the benchmarks.

@MatthewCroughan
Copy link
Collaborator

@emiflake Yes. Although if any maintainer that has push access to this github organization wants to test any PR, they can simply run this and they'll be able to test the PR in the same way you described. It's just not a button on the GitHub interface.

# get $rev from tip of branch from fork
git fetch origin $rev # This works even if it's on a fork
git push origin "$rev:refs/heads/tmpbranch"
git push origin :tmpbranch

@emiflake
Copy link
Collaborator Author

# get $rev from tip of branch from fork
git fetch origin $rev # This works even if it's on a fork
git push origin "$rev:refs/heads/tmpbranch"
git push origin :tmpbranch

Oh neat, good to know, for sure. But this still wouldn't run the benchmark-diff effect, since it's not on staging. That's what I'm asking about.

@MatthewCroughan
Copy link
Collaborator

@emiflake When making effects, I make it without the if statement to test that it works. Then, when I'm sure it works as expected, I add the if statement.

Copy link
Member

@L-as L-as left a comment

Choose a reason for hiding this comment

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

LGTM

@L-as L-as merged commit c9e5dd2 into staging Jan 24, 2022
@emiflake emiflake deleted the emiflake/document-plutarch-benchmark branch March 11, 2022 15:58
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.

None yet

3 participants