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

Feature request: allow specifying mainline #341

Closed
houserx-jmcc opened this issue May 10, 2023 · 9 comments
Closed

Feature request: allow specifying mainline #341

houserx-jmcc opened this issue May 10, 2023 · 9 comments
Labels
enhancement New feature or request prio: next Planning to work on this issue next

Comments

@houserx-jmcc
Copy link

Allow specifying a default mainline to use when cherry picking and if supplied use it, i.e. git cherry-pick 1234567 -m 1. Since this action can be used in very specific workflows (like my use case!), this could be a useful option to specify.

Example:

steps:
      - uses: actions/checkout@v3
      - name: Create backport pull requests
        uses: korthout/backport-action@v1
        with:
           default_mainline: 1
@korthout
Copy link
Owner

Hi @houserx-jmcc 👋 and thanks for raising this! I ran into a case where I also needed this. The pull requests were updated with the recent changes from the target branch with merge commits.

Could you share you're use-case/workflow? It may help me prioritize this feature.

@houserx-jmcc
Copy link
Author

Thanks! We are working through some changes to our development flows, so I wouldn't say we have an explicit path at the moment, but it seems that some way or another we find ourselves often needing to cherry pick upwards or downwards from a merge commit 🙃

@korthout
Copy link
Owner

korthout commented May 30, 2023

With #340 completed and released in 1.3.0, I'm deciding what to implement next. Are you still in need of this feature @houserx-jmcc?

@houserx-jmcc
Copy link
Author

A nice to have, but our initial need for it has waned a bit so no need to prioritize it above other things. Thank you for checking in!

@tdonohue
Copy link

tdonohue commented Aug 3, 2023

We've hit this problem ourselves... the backport-action fails anytime a developer updates their PR with one or more merge commits. Here's an example of a very small PR which fails to be backported because the second commit is a merge commit (developer updated their PR to latest code by merging main back into the PR): https://github.com/DSpace/dspace-angular/pull/2381/commits

Because we accept PRs from anyone via developer forks, we cannot stop the developer from updating their PR via a merge commit (or rebase the PR to remove the merge commit). Unfortunately, anytime they do so, this backport-action fails & we have to perform a manual cherry-pick instead.

So, I agree it'd be nice if there was a way for this action to somehow skip (or support) merge commits in a more automated manner!

@korthout
Copy link
Owner

korthout commented Aug 4, 2023

Thanks for sharing this @tdonohue. It helps me prioritise this issue knowing that more users are running into it. I can't promise when I'll find time for it with it being summer and all, but now this one is next on my list of improvements for backport-action.

@korthout korthout added the prio: next Planning to work on this issue next label Aug 4, 2023
@korthout
Copy link
Owner

korthout commented Aug 6, 2023

A first version of this is available for testing in korthout/backport-action@341-default-mainline, which supports a new default_mainline input.

Note that the only usable values at this time are:

  • default_mainline: '' (disabled)
  • default_mainline: 1 (use parent 1 as mainline)

When disabled, the behavior is as before, i.e. the action fails when it encounters a merge commit and reports about its failure. Using 1 as the default mainline means that all changes from the merge commit are cherry-picked to a new commit.

It is not yet possible to use 2 (or higher) as the default_mainline because the mainline flag is applied to the cherry-pick of each commit, incl. any non-merge commits. In this case, the action fails when a commit does not have that many parents. I've tried to keep the changes small and simple for now. I think we can do this in a separate feature request.

It is not yet possible to skip merge commits. That may be a useful behavior for default_mainline: 0. I think we can also split this to a separate feature request.

@tdonohue I'd love to know whether the default_mainline: 1 behavior solves the issue for you, or whether you have an explicit need for any of the other options I described. If the feature works for you as is then I'm happy to release this properly.

@tdonohue
Copy link

tdonohue commented Aug 8, 2023

@korthout : I gave it a try on our end. Unfortunately, the --mainline=1 flag to cherry-pick doesn't seem to work for our scenario. I suspect we need --skip for the merge commits (at least most of them), as they are providing no useful/new code to the backport PR. More details/logs at #372 (comment)

@korthout
Copy link
Owner

korthout commented Aug 9, 2023

Thanks for your feedback @tdonohue. It is very helpful.

I am now quite convinced that the common use case requires skipping the merge commits. We can continue this in a separate issue.

I also think that this feature request is no longer needed, so I'll close this issue. But, if anyone sees a use case for setting a default mainline then please let me know. Feel free to reopen this issue.

@korthout korthout closed this as not planned Won't fix, can't repro, duplicate, stale Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request prio: next Planning to work on this issue next
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants