flake: always run benchmark-diff effect

[emiflake]

Summary: Run benchmark-diff effect on all commits instead of only those on staging.
Essentially, this change is necessary to make the workflow work as intended by #123 (comment).

Now all future PRs (like this one) will actually run the effects and show them in the checks.

[MatthewCroughan]

This compares everything against staging. This means that if I make a PR to Master, it will actually lie and mislead me, and make me think it's comparing the performance of my PR, which it would not be. I do not think this should be merged.

[emiflake]

This compares everything against staging. This means that if I make a PR to Master, it will actually lie and mislead me, and make me think it's comparing the performance of my PR, which it would not be. I do not think this should be merged.

Fair point, I understand the concern. How's this, then. We'll do it when not on master. And assume all PRs follow the staging workflow and are supersets of staging.

[MatthewCroughan]

I don't see the point of doing that. Simply make a PR targeting staging, and you'll get a benchmark. There is no other purpose for this effect. All other data is not relevant. We do not have access to the merge-base via Nix until Hercules-CI 0.9 is released. Once that is released, we can do this for any branch, until then we cannot.

[srid]

Simply make a PR targeting staging, and you'll get a benchmark

You will not get a benchmark diff (against staging). See #173 for an example. And that's what this PR fixes.

[emiflake]

Simply make a PR targeting staging, and you'll get a benchmark.

Except, sadly that's not true. See bfd237b. This is on a PR for #173.

[MatthewCroughan]

Ah, okay. That makes sense. Adding the if statement was a mistake from me. It is the case that it will not run until it is merged, which is not correct.

[emiflake]

Ah, okay. That makes sense. Adding the if statement was a mistake from me. It is the case that it will not run until it is merged, which is not correct.

Yeah, although your concern is still valid. And this still will sort of be misleading in cases where it runs on branches off of master. Though sadly I don't think we can check if "HEAD came after staging" in a runIf, can we?

[srid]

Oh I hadn't noticed that the PR diff changed.