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.

Sign up for GitHub

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

Jump to bottom

benchmark workflow broken #93

Closed
2 tasks done
Uzlopak opened this issue Aug 31, 2023 · 3 comments · Fixed by #120
Closed
2 tasks done

benchmark workflow broken #93

Uzlopak opened this issue Aug 31, 2023 · 3 comments · Fixed by #120
Labels
bug Something isn't working

Comments

@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 31, 2023

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the issue has not already been raised

Issue

It seems that github.event.pull_request.head.repo.full_name is potentially incorrect. maybe it has to be github.event.pull_request.base.repo.full_name.
@Uzlopak Uzlopak added the bug Something isn't working label Aug 31, 2023
@Eomm
Copy link
Member

Eomm commented Sep 2, 2023

There should be both fields:
https://docs.github.com/en/webhooks/webhook-events-and-payloads#pull_request

@Uzlopak Uzlopak mentioned this issue Feb 20, 2024
Closed
1 task
@mweberxyz
Copy link
Contributor

mweberxyz commented Feb 20, 2024 •
edited
Loading

I believe I understand the issue, taking fastify/point-of-view#408 as an example, the filtered pull_request payload is:

CleanShot 2024-02-20 at 14 18 12@2x

But 408 is merging from mweberxyz/point-of-view fork into fastify/point-of-view, so the second actions/checkout@v4 should use the base instead -- so that the benchmarks run from fastify/pov#master, instead of mweberxyz/pov#master

@Uzlopak is that the gist of it?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Feb 20, 2024 •
edited
Loading

@mweberxyz

I think your assessment is right.

mweberxyz added a commit to mweberxyz/fastify-workflows that referenced this issue Feb 20, 2024
@mweberxyz
fix(plugins-benchmark-pr): ensure comparison benchmarks run against P…
edeb892 
…R base

Closes fastify#93

Previously, the second benchmark ran against the default branch of the (potentially forked) repo, which might not be in sync with the PR target repo.
@mweberxyz mweberxyz mentioned this issue Feb 20, 2024
Merged
1 task
Uzlopak pushed a commit that referenced this issue Feb 25, 2024
@mweberxyz
fix(plugins-benchmark-pr): run comparison benchmark against target (#120
9cb42e3 
)

* fix(plugins-benchmark-pr): ensure comparison benchmarks run against PR base

Closes #93

Previously, the second benchmark ran against the default branch of the (potentially forked) repo, which might not be in sync with the PR target repo.

* feature(plugins-benchmark-pr): factor up repos and refs and include in output

* refactor(plugins-benchmark-pr): inputs to dash case

For consistency

* refactor(plugins-benchmark-pr): consistency input and output naming, refs in step names

* refactor(plugins-benchmark-pr): consistency in step id

* docs(plugins-benchmark-pr): label should be removed from base

In my fork, the benchmark label was not removed with a 403 after the benchmark run, because it was trying to remove the label from PR in fastify repo — not the PR in forked repo.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(plugins-benchmark-pr): run comparison benchmark against target mweberxyz/fastify-workflows
3 participants
@mweberxyz @Uzlopak @Eomm