Release PR is closed when updating

baszalmstra commented 1 month ago

Bug description

In this repository Im making extensive use of release-plz. I absolute love it. However, for a little while now it appears that when release-plz updates the release PR it immediately closes it. I cannot find anything in the logs that suggests that release-plz is doing this.

Would you like to work on a fix? [n]

To Reproduce

I dont have isolated steps to reproduce this issue but I can show steps that exhibit this behavior in the aformentioned repository.

If you take a look at all the workflow runs of release-plz for the main branch: https://github.com/mamba-org/rattler/actions/workflows/release-rust.yaml

You can see that when PR 666 was merged release-plz create a new release PR. This is the workflow run: https://github.com/mamba-org/rattler/actions/runs/9186703934/job/25262873395

Release-plz notices changes in some of the crates and opens a release PR as can be seen in the logs:

2024-05-22T07:03:29.666711Z  INFO  opened pr: https://github.com/mamba-org/rattler/pull/668
    in open_pr
    in release_pr

release_pr_output: {"prs":[{"base_branch":"main","head_branch":"release-plz-2024-05-22T07-03-26Z","html_url":"https://github.com/mamba-org/rattler/pull/668","number":668}]}

However the next run of release-plz (the release pr was not merged), seems to update the release PR.


  2024-05-22T15:06:28.471231Z  INFO  updated pr https://github.com/mamba-org/rattler/pull/668
    in release_pr

release_pr_output: {"prs":[{"base_branch":"main","head_branch":"release-plz-2024-05-22T07-03-26Z","html_url":"https://github.com/mamba-org/rattler/pull/668","number":668}]}

Indeed the release PR also includes the changes of the last merged PR. However, it was also immediately closed. Reading the logs I can see no indication as to why this would happen.

Expected behavior

I would expect the release PR to not be closed. Manually reopening and merging the PR seems to be the workaround.

Screenshots

Environment

release-plz version: 0.3.68

Additional context

MarcoIeni commented 1 month ago

Hi Bas, thank you for you kind words ❤️

However, for a little while now it appears that when release-plz updates the release PR it immediately closes it.

I also experienced it once in release-plz repo itself.

I think it's due to https://github.com/MarcoIeni/release-plz/pull/1459 which fixed signed commits when release-plz updates the PR. Imo GitHub is closing the PR when we push the git changes, because I don't see this log line in the logs.

To fix this, I need a reproduction. I.e. a repository that I can fork, where you can observe this behavior by running release-plz release-pr locally.

If anybody is able to submit a reproduction it would be great, so I can fix this issue and also keep the signed commits fix.

If we can't find a reproduction we can revert https://github.com/MarcoIeni/release-plz/pull/1459 but then it would be hard to add the signed commit fix again, because we would be scared of introducing this bug again.

So I would really prefer to fix this bug instead of reverting.

I don't have time and resources to find a reproduction myself. If somebody can find a reproduction, it would be awesome 🙏

If you are too annoyed by this issue, you can use release-plz version 0.3.66 (action version v0.5.56) for now.

Thanks for understanding and sorry for the trouble, I hope this tradeoff (not reverting) I'm making doesn't annoy anyone too much. If yes, let's talk about it. 🙏

Pr0methean commented 1 month ago

I'm experiencing this too; examples are https://github.com/zip-rs/zip2/pull/147 and https://github.com/zip-rs/zip2/pull/152, both of which I had to manually reopen. You should be able to fork https://github.com/zip-rs/zip2 and tag releases without affecting real users, since the custom secret is always used for cargo publish and never for anything else. Or if you do need to cargo publish to repro this issue, you can change the crate name that's specified here: https://github.com/zip-rs/zip2/blob/master/Cargo.toml#L2

MarcoIeni commented 1 month ago

To avoid doing cargo publish you can do the following:

Are you able to fork the repo and give reproduction steps?

MarcoIeni commented 1 month ago

I was thinking. An alternative to the current flow, is to commit the changes on top of the previous commits, instead of force pushing. What do you think? 🤔 This would simplify the logic and make the history of the pr clearer, making it easier to understand when a breaking change was introduced. I'm not sure this is a good idea, because Dependabot and similar tools work differently (they force push). I'm curious what do you all think.

Pr0methean commented 1 month ago

I'm in favor, but would the CHANGELOG still be well-defined in that case?

baszalmstra commented 1 month ago

Sounds good to me too!

MarcoIeni commented 1 month ago

I'm in favor, but would the CHANGELOG still be well-defined in that case?

The content of the pr doesn't change imo. What's your concern? 🤔

MarcoIeni commented 1 month ago

I understood why this issue is happening. GitHub closes the PR if is "empty", i.e. there are no commits that differ from main. As a fix, we could maybe push an empty commit and then remove it with some git magic.

The approach I described without force-push requires more thought because in this case we need to understand how to deal with merge conflicts (force push solves this issue).

So no reproduction needed, I'll try to solve this issue in the following days 👍

MarcoIeni commented 1 month ago

Btw if anybody wants to give this a try, feel free. Pr welcome

MarcoIeni commented 1 month ago

As a fix, we could maybe push an empty commit and then remove it with some git magic.

This is not possible, because as soon as we force push, the commit created with github api becomes unverified.

So maybe we need to stop force pushing 🤔

My guess is that instead of force-pushing, we need to use GitHub API to push the new changes.

MarcoIeni commented 1 month ago

I read renovate's code. It seems they use the GitHub REST API to do the force push.

So it seems we have two options:

try to use renovate's approach and keep force pushing (not sure if this would work)
don't force-push anymore. But rather create a new commit every time there's an update

cc @zvolin as you might be interested in this discussion, and it would be great to hear your opinion 🙏

zvolin commented 1 month ago

Interesting, I'll read through it and see. I remember I was testing this with running release-plz from shell and it worked perfectly fine here. I'm not sure why this shouldn't be the case when running from CI, or maybe I made something differently :thinking:

zvolin commented 1 month ago

I don't like that idea but a quick work around could be to just re-open the PR instantly with github cli after the force push and commit

zvolin commented 1 month ago

I just checked and in lumina we also got the release pr closed. ~What's strange is that it force-pushed the branch but it looks like chore: release commit didn't happen~. Ah the commit is on branch, it's not listed on the closed PR

zvolin commented 4 weeks ago

Committing with rest api doesn't seem to get the Verified status. I haven't heard of the renovate before, but if this tool has it, then it's likely it's because it's some app/bot with integrated account? (I haven't used those too).

I ran this test. create some branch with some commit, push

git checkout -b abc
echo foo > README.md
git add -u && git commit -m foo && git push origin abc

create a new commit with some changes

echo bar > README.md
git add -u && git commit -m bar

get the tree of the latter commit. Tree is basically the 'state' part of commit, all blobs etc.

git cat-file -p "$(git rev-parse HEAD)"

tree 29aa1bf150e8ce459bbb2ccef4fbaa0b15701ce8
parent ce97bd268c6281d4b59a64efa8adcb579a6e04de
author zvolin <mac.zwolinski@gmail.com> 1717940382 +0200
committer zvolin <mac.zwolinski@gmail.com> 1717940382 +0200

bar

store the tree object somewhere in github

git push origin 29aa1bf150e8ce459bbb2ccef4fbaa0b15701ce8:refs/release-plz/trees/29aa1bf150e8ce459bbb2ccef4fbaa0b15701ce8

create new commit with gh rest api, using this tree but having master as parent instead of the foo commit. create-a-commit

curl -L \
  -X POST \
  -H "Accept: application/vnd.github+json" \
  -H "Authorization: Bearer $TOKEN" \
  -H "X-GitHub-Api-Version: 2022-11-28" \
  https://api.github.com/repos/zvolin/test-release-plz/git/commits \
  -d '{
    "message":"my commit message",
    "parents":["503fc8faaef49d115884c2b36eb4cbe117e1b935"],
    "tree":"29aa1bf150e8ce459bbb2ccef4fbaa0b15701ce8" 
  }'

{
  "sha": "4ec02c28533b98720c7f33c7cbec93d6da2d5c2e",
  "node_id": "C_kwDOLDtSTtoAKDRlYzAyYzI4NTMzYjk4NzIwYzdmMzNjN2NiZWM5M2Q2ZGEyZDVjMmU",
  "url": "https://api.github.com/repos/zvolin/test-release-plz/git/commits/4ec02c28533b98720c7f33c7cbec93d6da2d5c2e",
  "html_url": "https://github.com/zvolin/test-release-plz/commit/4ec02c28533b98720c7f33c7cbec93d6da2d5c2e",
  "author": {
    "name": "Maciej Zwoliński",
    "email": "mac.zwolinski@gmail.com",
    "date": "2024-06-09T14:07:27Z"
  },
  "committer": {
    "name": "Maciej Zwoliński",
    "email": "mac.zwolinski@gmail.com",
    "date": "2024-06-09T14:07:27Z"
  },
  "tree": {
    "sha": "29aa1bf150e8ce459bbb2ccef4fbaa0b15701ce8",
    "url": "https://api.github.com/repos/zvolin/test-release-plz/git/trees/29aa1bf150e8ce459bbb2ccef4fbaa0b15701ce8"
  },
  "message": "my commit message",
  "parents": [
    {
      "sha": "503fc8faaef49d115884c2b36eb4cbe117e1b935",
      "url": "https://api.github.com/repos/zvolin/test-release-plz/git/commits/503fc8faaef49d115884c2b36eb4cbe117e1b935",
      "html_url": "https://github.com/zvolin/test-release-plz/commit/503fc8faaef49d115884c2b36eb4cbe117e1b935"
    }
  ],
  "verification": {
    "verified": false,
    "reason": "unsigned",
    "signature": null,
    "payload": null
  }
}

It worked, but as you can see it's not Verified. 4ec02c28533b98720c7f33c7cbec93d6da2d5c2e

We can test the force-push anyway, update the abc ref. update-a-reference

curl -L \                   
  -X PATCH \
  -H "Accept: application/vnd.github+json" \
  -H "Authorization: Bearer $TOKEN" \
  -H "X-GitHub-Api-Version: 2022-11-28" \
  https://api.github.com/repos/zvolin/test-release-plz/git/refs/heads/abc \
  -d '{"sha":"4ec02c28533b98720c7f33c7cbec93d6da2d5c2e","force":true}'

{
  "ref": "refs/heads/abc",
  "node_id": "REF_kwDOLDtSTq5yZWZzL2hlYWRzL2FiYw",
  "url": "https://api.github.com/repos/zvolin/test-release-plz/git/refs/heads/abc",
  "object": {
    "sha": "4ec02c28533b98720c7f33c7cbec93d6da2d5c2e",
    "type": "commit",
    "url": "https://api.github.com/repos/zvolin/test-release-plz/git/commits/4ec02c28533b98720c7f33c7cbec93d6da2d5c2e"
  }
}

We could now remove the ref for the tree that we created to clean up.

So unfortunately commits created this way don't have the signature generated automatically for an authorized user but rather requires providing a pgp signature in api call, which has it's known drawbacks. I was hoping we could get rid of the graphql part when committing and only use the rest api all the way...

We could solve the issue by force-pushing to github in this way:

creating new branch from target branch, say "{release-branch}-tmp"
committing via graphql to this new branch
updating the PR's branch ref to be this new commit
deleting temporary branch

I'll see how bad does it look in a code

MarcoIeni commented 4 weeks ago

Thanks for testing this 🙌 🙏

We could solve the issue by force-pushing to github in this way

Yeah, that could work! Maybe let's attach a random string to the target branch. I.e. {release-branch}-tmp-{random} so that we avoid collisions across parallel release-plz runs 👍

I'll see how bad does it look in a code

Thanks ❤️

zvolin commented 4 weeks ago

I've just noticed that I copy pasted commands from docs in older versions of github api. Nevertheless I've tested also without -H "X-GitHub-Api-Version: 2022-11-28" \ in curl commands and their results were the same

MarcoIeni commented 3 weeks ago

Thanks Maciej for the fix 🙌

zvolin commented 3 weeks ago

I remember I was testing this with running release-plz from shell and it worked perfectly fine here. I'm not sure why this shouldn't be the case when running from CI, or maybe I made something differently 🤔

I think it worked for me then because I added commit on master and didn't push it to github before running release-plz release-pr again

MarcoIeni commented 3 weeks ago

From my understanding release-plz wasn't always closing the pr. Imo github takes a bit of time before closing the pr automatically. If release-plz commits before this interval, then the pr is not closed by github.

Anyway, now it should be solved!

MarcoIeni / release-plz