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.

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

CHANGELOG.md conflicts #233

Closed
SubJunk opened this issue Jan 17, 2024 · 22 comments
Closed

CHANGELOG.md conflicts #233

SubJunk opened this issue Jan 17, 2024 · 22 comments
Milestone

Comments

@SubJunk
Copy link

SubJunk commented Jan 17, 2024

I have run into a situation where there is almost always a conflict in CHANGELOG.md after I merge a PR that uses this action. Dependabot is unable to recreate the branch automatically, because Dependabot stops doing that when another actor pushes a commit.
Is there a recommendation for how to resolve this automatically? For now I have to leave a comment like @dependabot recreate in each PR it happens.
An example is at UniversalMediaServer/UniversalMediaServer#4364

@dangoslen
Copy link
Owner

Hey @SubJunk

Thanks for opening this issue. I have the same conflicts for PRs in this repo itself. I've been able to just use the @dependabot recreate without a big issue, but I agree it isn't a great experience.

I have two general ideas here

  1. Utilize groups to reduce the number of PRs and perform updates all at once. This action supports multi-dependency updates too
  2. Utilize the activationLabel and create a workflow to run on a label you add once you are ready to merge (and utilize something like auto-merge in the workflow) to update the changelog and merge it right after. This would allow dependabot rebasing update to happen automatically as you wouldn't add a commit until right before merging. You would essentially mark the label for this action to be automerge instead of dependabot

I personally think Option 2 is a good path to explore - and I might prototype for myself in this repo if I have time this week.

@SubJunk
Copy link
Author

SubJunk commented Jan 24, 2024

Thanks for your reply. Those are good ideas. I tried to automate it fully here UniversalMediaServer/UniversalMediaServer@881cc65 which is an action that runs every time a PR is merged. It looks through all PRs that are still open, and adds a comment and label if they are conflicted. I make it leave the comment @dependabot rebase which worked, but Dependabot did not like the permissions.

You can see at UniversalMediaServer/UniversalMediaServer#4364 (comment) that the comment and label worked, but Dependabot replied with:

Sorry, only users with push access can use that command.

It's possible that we could give that bot the access that Dependabot needs and it would work.

There is another limitation which is that it would do it for all PRs, not just Dependabot ones. It's not a practical problem because Dependabot will ignore PRs that it did not create, but it is comment spam. It still feels like a good approach to me if we can give the bot push access though.

Edit: I will try again with this approach because it feels like it is almost working

@dangoslen
Copy link
Owner

@SubJunk that is a neat approach!

I do think Dependabot does rebasing automatically, it just might take time for it to propagate without a manual comment.

I am also in the process of extending this action to require a list of labels by which to execute (new input as activationLabels). This should allow you to have a workflow that requires some "automerge" label along with dependabot. I haven't cut a release yet, but it is in main if you wanted to try that out. The workflow is here

@SubJunk
Copy link
Author

SubJunk commented Jan 25, 2024

@dangoslen you're right that Dependabot usually does it automatically, but it doesn't do it if anyone else has committed to the branch, so it doesn't happen here

I have this workflow working now though! I had to create a custom access token, so now it leaves a comment as me. Actually a bit weird since it is indistinguishable from my real comments, but cool. e.g. UniversalMediaServer/UniversalMediaServer#4426 (comment)

I think I will request a feature from that other Action to allow filtering for PR authors/bots. If I can make it only comment on Dependabot PRs it will be perfect
Edit: I made that feature request at eps1lon/actions-label-merge-conflict#113

@dangoslen
Copy link
Owner

Very cool!

One of the ideas I was trying to mention earlier was that by utilizing an auto-merge tool, this action could trigger at the "end" of the PR lifecycle, allowing Dependabot to rebase for you.

Take a look at the PR in #244 as it will allow activating/running this action based on the presence of multiple labels vs. just one.

@SubJunk
Copy link
Author

SubJunk commented Jan 27, 2024

@dangoslen thanks for your reply, so instead of clicking the merge button, I apply the label, which triggers this changelog action followed by the automerge action? I like that, I think it could save time by creating fewer conflicts. I will try it

@dangoslen
Copy link
Owner

dangoslen commented Jan 29, 2024

Exactly right. I've set up that same workflow for this repo. I'll see how it all works tomorrow when my weekly dependabot PRs come through.

@dangoslen
Copy link
Owner

@SubJunk

Here is an example of this idea in action: #246

On that PR, this action is configured to run when both the automerge label and the dependabot label are present. As you can see in the history, I added the automerge label only when I was ready to merge. Once I did, it updated the changelog, passed tests, and merged it into main. I had also merged another dependabot PR in the same way a few minutes prior, and I didn't encounter any merge conflicts

@dangoslen
Copy link
Owner

I've just released version 3.8.0 to add the multiple labels.

At this point, I think a good idea to resolve/close this issue is to write up some example documentation for how to solve this. I don't think this action has the capability of totally solving the merge conflict problem, but I do think there are strategies (like the ones we've discussed) that can aid in conflict avoidance/prevention to help users have a better experience.

Does that sound like a reasonable compromise @SubJunk ?

@wcampbell0x2a
Copy link

I added a new activationLabels: dependabot-reviewed here: https://github.com/wcampbell0x2a/backhand/pull/466/files.

However, I tried using this in this MR: wcampbell0x2a/backhand#464, and I couldn't the Action to trigger. Help would be appreciated.

@stevehipwell
Copy link

@dangoslen it seems like this is related to #231?

@dangoslen
Copy link
Owner

Hi @wcampbell0x2a. Thanks for letting me know

Looking at the second PR you linked and the actions that ran, I noticed that in the entire workflow was skipped rather than this action not running itself. I noticed you have a if check for the author on the workflow. Could you try removing that if and see what happens? If I understand the rest of that workflow, since the only real activity would be attempting to run this action (which will only run on specific labels) and then committing changes if they exist, it seems safe to remove that check.

Let me know how else I can help!

@dangoslen
Copy link
Owner

@stevehipwell I do agree this could be remedied by running the updates on main vs. the PR itself. I haven't had a chance to dig into it further though.

@wcampbell0x2a
Copy link

Thanks for the help, that makes sense!

Another issue: https://github.com/wcampbell0x2a/backhand/actions/runs/7823808152/job/21345338507?pr=469

Run dangoslen/dependabot-changelog-helper@ffabc6ebe06fd7717ad1929945c8b95a4d9e7ba0
Label 'dependabot' not found in pull request labels

But I use C-dependencies, dependabot-reviewed

@dangoslen
Copy link
Owner

@wcampbell0x2a I'll look into this. I have a theory that I incorrectly am handling the deprecated input. If so, I'll try to get a 3.8.1 version out today

@dangoslen
Copy link
Owner

@wcampbell0x2a I have a PR with the fix ready. Would you be willing to verify it in your workflow to make sure it resolves your issue? #250

@wcampbell0x2a
Copy link

@wcampbell0x2a I have a PR with the fix ready. Would you be willing to verify it in your workflow to make sure it resolves your issue? #250

Sure. How do I do this?

@dangoslen
Copy link
Owner

You should be able to adjust the workflow to point to that PR dangoslen/dependabot-changelog-helper@fix-defaults

@wcampbell0x2a
Copy link

@dangoslen
Copy link
Owner

Ah. So while you committed that change to the workflow, since its a pull_request_target, it won't run with the updated workflow until the workflow is the target branch of a PR.

I am pretty confident it will fix your issue. I'll go ahead and cut a 3.8.1 release today and we can go from there. If the issue persists, I'd advocate for opening a new issue so as to not keep sending notifications to others on this thread.

@wcampbell0x2a
Copy link

Ah. So while you committed that change to the workflow, since its a pull_request_target, it won't run with the updated workflow until the workflow is the target branch of a PR.

I am pretty confident it will fix your issue. I'll go ahead and cut a 3.8.1 release today and we can go from there. If the issue persists, I'd advocate for opening a new issue so as to not keep sending notifications to others on this thread.

Fine by me. I'll make sure to let you know if it works once I update to that version.

@dangoslen
Copy link
Owner

I cut 3.8.1 yesterday. If you experience more issues, please open a new issue and we will figure it out.

@dangoslen dangoslen added this to the v3.8.1 milestone Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants