search

lorenzwalthert / touchstone

Smart benchmarking of pull requests with statistical confidence
https://lorenzwalthert.github.io/touchstone
Other
53 stars 7 forks source link

possibility of turning off notification on every push? #111

Closed njtierney closed 2 years ago

njtierney commented 2 years ago

Hello!

I was wondering if it might be possible to turn off notifications on every push, and instead perhaps ask the bot for a speed comparison when wanted?

lorenzwalthert commented 2 years ago

Quick answer: Currently not supported. You can do it in your GitHub Actions workflow file yourself, most likely be following the r-lib/actions example for PR commands. E.g. to style the code in a PR, a contributor can comment with /style. To adapt the default GitHub Action workflow, we need:

Long answer: I guess @assignUser when touchstone::use_touchstone(), the user should decide if they want to run touchstone on every PR or only on request. I think we can enable both in one workflow file and maybe the user can just comment out like this to enable/disable the automatic benchmarking:

on:
  issue_comment:
    types: [created]
# COMMENT OUT TO DEACTIVATE RUNNING ON PR: START
  pull_request: 
    types: [created]
# COMMENT OUT TO DEACTIVATE RUNNING ON PR: END
...

    if: ... ???

Did not have time to think about how the if condition needs to look like exactly to work for both cases.

assignUser commented 2 years ago

yep that should be possible, I'll have a look.

assignUser commented 2 years ago

A quick update: this is possible but the action triggered by the issue command does not have the HEAD in it's payload so we have to get that manually. I'll look into that today.

assignUser commented 2 years ago

This was a bit more involved than anticipated. @njtierney I have opened a pr with the changes to your workflow file to enable this: https://github.com/greta-dev/greta/pull/517 Let me know if this is what you where looking for!

@lorenzwalthert While working on this I noticed that benchmarks from external forks would use the origin base branch and not the upstream base branch, this could lead to misleading benchmark results e.g. if the fork is not up to date. I implemented logic in the "manual-trigger" branch to resolve this issue. (with an option to force benchmarking against upstream in "test" pr within forks)

njtierney commented 2 years ago

Thanks, @assignUser ! Sorry for the delay getting back to you, just sorting my life out after some time away on holiday.

Can I ask, what is the command to trigger the benchmark loading on PR?

Thanks so much for implementing this feature, really appreciate it! :)

njtierney commented 2 years ago

Ah, wait, I see, the command is /benchmark, correct?

assignUser commented 2 years ago

@njtierney Hey, no worries! 🏖️ Yes exactly /benchmark you could also change it to something else if you'd like by replacing that in the workflow.file

njtierney commented 2 years ago

Awesome! Thank you!