search

preactjs / compressed-size-action

GitHub Action that adds compressed size changes to your PRs.
https://github.com/marketplace/actions/compressed-size-action
MIT License
602 stars 84 forks source link

Compare against the state of the target branch at the moment the branch was created. #10

Closed youknowriad closed 4 years ago

youknowriad commented 4 years ago

Hi and thanks for this great github action.

At the moment, if you want to have the right diff numbers, the PR branch need to be fresh compared to the target branch (rebased). In a fast-moving repository, this can be hard to ensure consistently, so you end up with numbers that doesn't make a lot of sense.

I was wondering if it's possible to diff against the PR branch minus its commits (the hash just before the first commit of the PR branch) instead of comparing against the current state of the target branch.

Thanks.

developit commented 4 years ago

@youknowriad I appreciate you looking into this! I'm pretty new to Github actions too. I'm a little bit confused here, because I was actually under the impression that pr.base.sha was the parent commit on which the PR is based, not the latest commit to the target branch. If that isn't the case, I bet there's a correct sha somewhere in Github's context object.

FWIW in our usage of this action on Preact I can't say I've run into the issue of master being already ahead of a PR by the time it gets opened - maybe we're not a fast moving enough repository to trigger it? I guess this would also be affected by how long a branch has been up prior to the PR being created, since this action runs on the PR rather than the branch.

developit commented 4 years ago

If you have an example PR where the numbers end up being wrong that would be super useful. We should be able to pull the entire Github context object out via the API and look at what's available.

youknowriad commented 4 years ago

For context, we're using this in the Gutenberg repository https://github.com/WordPress/gutenberg/

I definitely saw weird numbers on several PRs and after a rebase of the PR, the numbers were correct again. I'll keep an eye and update here as I find examples.

In my attempts to fix, I tried using the pr.base.sha exclusively as I was assuming that it was the parent commit too but the numbers were wrong too, so I'm a bit confused too now :)

developit commented 4 years ago

@youknowriad perhaps we could try adding an input option like use-pr-base-sha: true to the action to test out different hashes?

youknowriad commented 4 years ago

@developit Thanks for considering that. I just did more tests with this PR specifically https://github.com/WordPress/gutenberg/pull/20528 and I can't seem to reproduce any issues even after letting that PR sit for a few days.

I'm confused and I don't know exactly what happened originally. So, for now, I'm going to just close the issue and sorry for the waisted time.