Action does not properly handle rebases

[nwiltsie]

I added a plantuml figure (among many other revisions) to this repo while a dependabot PR was in place. After merging all other branches I went to rebase the dependabot branch, @dependabot rebase and it failed the plantuml action: fatal: Invalid revision range d1c56e3ab79e4534a825b8e19f3c5d1bf339edd9..9d914328d0782e17f7c41257e9cc2131734c174b
Error: Process completed with exit code 128.. I tried rebase a second time which also failed, then @dependabot recreate which was successful.

Is this something that shouldn't happen and can be fixed. Or should I just add the situation to the instructions at Dependabot Pull Request Protocol

Originally posted by @sorelfitzgibbon in uclahs-cds/pipeline-generate-SQC-BAM#22 (comment)

[nwiltsie]

Here are the two unrelated commmits:

uclahs-cds/pipeline-generate-SQC-BAM@d1c56e3
uclahs-cds/pipeline-generate-SQC-BAM@9d91432

[aholmes]

From Nick's comment:

@sorelfitzgibbon In the best case, this came up because main acquired a PlantUML image between when this PR was opened and when you rebased it, which is a relatively uncommon case.

This is essentially what happened.

1. The checkout action clones a repository in whatever state it's in when it does so
2. The PlantUML action is given a Git commit SHA that represents the change prior to the commit that triggered the action. This was d1c56e3ab79e4534a825b8e19f3c5d1bf339edd9 (the now-invalid commit).
3. The PlantUML action attempts to find differences between the previous, now invalid, commit and the current HEAD.
4. The action errors because the commit SHA no longer exists.

We can see the steps play out by examining the comments in this PR and the action log:

1. At 2024-03-08T15:52:03Z Sorel told Dependabot to rebase.
2. At 2024-03-08T15:52:26Z Dependabot force-pushed commit 9d914328d0782e17f7c41257e9cc2131734c174b, invalidating d1c56e3ab79e4534a825b8e19f3c5d1bf339edd9.
3. At 2024-03-08T15:52:28Z, the failed action started.
4. At 2024-03-08T15:52:40Z the repository is "cloned."
5. At 2024-03-08T15:52:41Z the action errored.

The problem seems to be with step 2, in that the previous commit SHA GitHub gives to the action is no longer valid.

[nwiltsie]

@aholmes does that mean that this will happen every time a rebase occurs in a repository with this action and an image?

[aholmes]

I'm wondering about that. I haven't seen this before, but this observation does seem like it would occur every time. I'm not certain whether I missed a specific detail.

[nwiltsie]

Wait, why did the workflow even run in the first place?

---
name: PlantUML Generation

on:
  push:
    paths:
      - '**.puml'
  workflow_dispatch:

The dependabot PR shouldn't have included any PlantUML files...

[aholmes]

The rebase includes PUML files (and the PlantUML action addition itself).

[aholmes]

I'm considering changing the action to test whether the previous commit SHA is valid and, if it isn't, defaulting to the "every PUML file" mode rather than attempting to include only the PUML files since the last push.

[nwiltsie]

There's some weirdness bandying about here - the actual commit itself (uclahs-cds/pipeline-generate-SQC-BAM@9d91432) includes no PlantUML files.

Comparing the old/abandoned commit and the new one (https://github.com/uclahs-cds/pipeline-generate-SQC-BAM/compare/d1c56e3ab79e4534a825b8e19f3c5d1bf339edd9..9d914328d0782e17f7c41257e9cc2131734c174b) shows that a PlantUML file was added, but that's not part of this pull request.

I would have thought the workflow filter would care about the changes relative to the mergee-branch (https://github.com/uclahs-cds/pipeline-generate-SQC-BAM/compare/0a5aae07cdb53c7da6977f682f46c0d73f3f5ca0..9d914328d0782e17f7c41257e9cc2131734c174b).

Weird! I agree though that this is a super-edge case where a new PlantUML file appears in the commits jumped by a rebase. I also agree that including a valid SHA check is fair.

[aholmes]

Yeah, I'm not sure I can speak to exactly what's going on there beyond speculation. Observing this shows that there is the possibility that Actions treats a rebase as a big chunk of changes rather than just a rebase. Whether that's true, it should be safe for us to act like that's the case, and code accordingly.

[nwiltsie]

Ahh, okay, this isn't a "pull request" workflow, it's just a workflow. There's no concept of a "mergee-branch" (whatever the proper name for that is) - it only has reference to the last known commit, hence why the PlantUML file appears new.

[aholmes]

Ah - good point. Yes, this Action considers everything from the last push. So the issue is there. :)

[aholmes]

@nwiltsie suggested using https://docs.github.com/en/webhooks/webhook-events-and-payloads#push and the github.event.commits.added / modified arrays. This would also resolve a significant portion of #2.