feat: allow backporting to remote repository #405

tasso94 · 2023-12-19T09:29:11Z

This pull request is still a work in progress, but I wanted to share my first iteration to get feedback early on:

Whether it generally makes sense and if you want to merge it with your upstream.
If the technical design of the feature makes sense.

User story

As a user, I maintain my master branch and the backport branches in two separate repositories to hide my backport branches from the public.

Open tasks

I think the following topics still need to be addressed (is there even more I might not know yet?):

Improve the naming of newly introduced config properties (I'm open to any ideas).
Write documentation.

korthout · 2023-12-21T10:25:58Z

Thanks @tasso94 🙇 I'm currently enjoying some time off, so it may take me some more time until I review this. But at first glance, this looks amazing!

korthout

Thanks for this massive contribution @tasso94 🙇 This will be super useful!

I've had a closer look at the changes and have a few comments, suggestions, and questions. I've used an emoji review code to clarify my comments. Please have a look.

src/backport.ts

action.yml

src/git.ts

action.yml

tasso94 · 2024-01-15T09:42:50Z

Hi @korthout,

Thanks for your review feedback, and I'm sorry for my late response.

Right now, I'm a bit busy but I'll try to have a look soon.

Stay tuned!

Best,
Tassilo

tasso94 · 2024-01-25T18:10:59Z

Hi @korthout,

Thank you for providing your input. I applied your review hints. 🙂 👍
Could you please review my changes?

I know that I still need to document the new properties. If you don't have any further remarks regarding the naming or characteristics of the properties, I will add the documentation.

Is there anything else that is required from my side to make this PR merge-ready? Do you want me to contribute tests?

Best,
Tassilo

korthout · 2024-01-31T07:27:08Z

Thanks for the great work @tasso94. I want to give this the proper attention it deserves, but I've been too busy at work as well as in my personal life (with an international move coming up ahead) to review this. Please know that I haven't forgotten about this. I really appreciate this effort and see it as a big improvement to the action. A full review will follow.

tasso94 · 2024-02-20T10:31:28Z

Hi @korthout,

Do you already have a plan for when you review my contribution? 🙂

Best,
Tassilo

korthout

Thanks @tasso94 for the improvements. Sorry for taking so long 🙇 I want to give this the attention it deserves, but it was hard to find the time. Now, I finally had a chance to give it a spin, and I'm happy to report that it works! It's really cool to see it in action! 🚀

Some notes:

❌ There's a small mistake in one of the logs. It refers to origin, but actually pushed to target. This should be changed before merging. See below.
💭 I've tried to think a bit more on our usage of the word remote to mean the other repository. GitHub uses upstream as the name for the original repository from the perspective of a forked repository. In that same analogy, what we're currently calling remote is actually the downstream repository. Changes from the original repository are backported to a downstream repository (often a fork). However, I worry that downstream is too unfamiliar to be used here. So, perhaps remote is still the best wording. I'd love your input on this.
📝 since the default GITHUB_TOKEN does not have write access to the other repository, it's mandatory to use a PAT for the token input of the checkout action and the github_token input of the backport-action itself. We should document this alongside these inputs.

Example of log wrongly referring to origin

Push branch to origin
git push --set-upstream target backport-365-to-case5-backport-target

In addition to my ramblings on naming things and the small logging mistake, I only have a few minor suggestions. Please have a look 🙇

👍 I'll be happy to merge this with a bit of documentation

EDIT: Regarding tests, we can do without for now. It would be cool to expand https://github.com/korthout/backport-action-test with some test case for this eventually. But it's fine to only cover this manually for now.

src/backport.ts

tasso94 · 2024-03-06T13:43:43Z

Hi @korthout,

Thank you for the review and for bringing up the naming. 🙂 👍

I prefer downstream to remote since the latter is super generic, whereas the former is self-explanatory.

What do you think? Should we move forward with downstream? 🤔

korthout · 2024-03-11T08:29:44Z

Let's go for it!

korthout · 2024-03-27T06:58:28Z

@tasso94 is there anything I can do to help get this merged?

action.yml

tasso94 · 2024-04-11T15:31:54Z

Hi @korthout,

I was quite busy in the last couple of weeks.
I'll did some final touches and incorporated your latest feedback. I'll have a final look Tomorrow. Thanks for your patience! 🙂 👍

Best,
Tassilo

tasso94 · 2024-04-30T12:18:59Z

Hi @korthout,

Friendly reminder to review my latest changes 🙂 👍

Best,
Tassilo

If git fetch fails we should clarify which remote was used to fetch from.

korthout

🙇 Sorry for taking so long. I was in the process of moving internationally. I'm trying to catch up again now.

❤️ Thanks for the amazing work on this! I love the solutions you found and I'm happy with the naming.

I've taken the liberty to adjust some last small things, but nothing major.

Let's merge this! 🚀 Please try it out in your repository and let me know if you run into any issues. In time, we can promote the inputs from their experimental status.

LGTM 👍

korthout · 2024-05-03T15:43:40Z

To try this out you can either use:

korthout/backport@main
korthout/backport@042b2d4
korthout/[email protected]

mvayngrib · 2024-05-03T16:36:05Z

nice, thank you guys!

mvayngrib

left a few small suggestions

mvayngrib · 2024-05-03T16:37:17Z

src/backport.ts

+      this.config.experimental.downstream_owner ?? undefined;
+  }
+
+  shouldUseDownstreamRepo(): boolean {


nit: if this doesn't need to be used externally, better make it private

mvayngrib · 2024-05-03T16:37:58Z

src/backport.ts

+    return !!this.downstreamRepo;
+  }
+
+  getRemote(): "downstream" | "origin" {


same, could be private

korthout · 2024-05-09T13:53:32Z

Thanks for the notes @mvayngrib. I'd welcome any pull request with improvements 🙇

Did you already have a chance to try out this functionality? Any feedback is welcome.

mvayngrib · 2024-05-09T20:49:58Z

thanks @korthout, i haven't yet, but will share feedback when i do 👍

tasso94 force-pushed the main branch 3 times, most recently from 651a8d9 to 422a91f Compare December 20, 2023 10:18

korthout linked an issue Dec 21, 2023 that may be closed by this pull request

ability to backport a PR to any repo, not necessarily a fork #384

Closed

korthout requested changes Jan 4, 2024

View reviewed changes

src/backport.ts Outdated Show resolved Hide resolved

action.yml Outdated Show resolved Hide resolved

action.yml Outdated Show resolved Hide resolved

src/git.ts Outdated Show resolved Hide resolved

action.yml Outdated Show resolved Hide resolved

tasso94 force-pushed the main branch 2 times, most recently from 3add836 to eb924e6 Compare January 25, 2024 18:17

tasso94 requested a review from korthout January 25, 2024 18:18

tasso94 force-pushed the main branch from eb924e6 to dc2f833 Compare January 25, 2024 19:38

korthout mentioned this pull request Feb 25, 2024

case(5): Rebase and merge korthout/backport-action-test#365

Merged

korthout requested changes Feb 25, 2024

View reviewed changes

src/backport.ts Outdated Show resolved Hide resolved

src/backport.ts Outdated Show resolved Hide resolved

src/backport.ts Outdated Show resolved Hide resolved

korthout reviewed Mar 30, 2024

View reviewed changes

action.yml Outdated Show resolved Hide resolved

tasso94 added 2 commits April 12, 2024 10:15

feat: allow backport to remote repository

6510cd4

apply review hints

4586af2

tasso94 force-pushed the main branch from dc2f833 to 0824057 Compare April 12, 2024 08:37

apply review hints

02d15db

tasso94 force-pushed the main branch from 0824057 to 02d15db Compare April 12, 2024 10:09

tasso94 requested a review from korthout April 12, 2024 11:22

tasso94 marked this pull request as ready for review April 12, 2024 11:23

fix: log remote in git fetch errors

79396b7

If git fetch fails we should clarify which remote was used to fetch from.

korthout added 2 commits May 3, 2024 17:34

docs: clarify experimental downstream inputs

12c1d74

dist: build new version

3c984b3

korthout approved these changes May 3, 2024

View reviewed changes

korthout merged commit 042b2d4 into korthout:main May 3, 2024
1 check passed

mvayngrib reviewed May 3, 2024

View reviewed changes

korthout mentioned this pull request May 9, 2024

Add conflict_resolution input #417

Merged

5 tasks

yanavasileva mentioned this pull request May 16, 2024

chore(backport): use with korthout official upstream camunda/automation-platform-github-actions#18

Merged

korthout mentioned this pull request May 26, 2024

ability to backport a PR to any repo, not necessarily a fork #384

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow backporting to remote repository #405

feat: allow backporting to remote repository #405

tasso94 commented Dec 19, 2023 •

edited

Loading

korthout commented Dec 21, 2023

korthout left a comment

tasso94 commented Jan 15, 2024

tasso94 commented Jan 25, 2024

korthout commented Jan 31, 2024

tasso94 commented Feb 20, 2024

korthout left a comment •

edited

Loading

tasso94 commented Mar 6, 2024

korthout commented Mar 11, 2024

korthout commented Mar 27, 2024

tasso94 commented Apr 11, 2024

tasso94 commented Apr 30, 2024

korthout left a comment

korthout commented May 3, 2024 •

edited

Loading

mvayngrib commented May 3, 2024

mvayngrib left a comment

mvayngrib May 3, 2024

mvayngrib May 3, 2024

korthout commented May 9, 2024 •

edited

Loading

mvayngrib commented May 9, 2024

feat: allow backporting to remote repository #405

feat: allow backporting to remote repository #405

Conversation

tasso94 commented Dec 19, 2023 • edited Loading

User story

Open tasks

korthout commented Dec 21, 2023

korthout left a comment

Choose a reason for hiding this comment

tasso94 commented Jan 15, 2024

tasso94 commented Jan 25, 2024

korthout commented Jan 31, 2024

tasso94 commented Feb 20, 2024

korthout left a comment • edited Loading

Choose a reason for hiding this comment

tasso94 commented Mar 6, 2024

korthout commented Mar 11, 2024

korthout commented Mar 27, 2024

tasso94 commented Apr 11, 2024

tasso94 commented Apr 30, 2024

korthout left a comment

Choose a reason for hiding this comment

korthout commented May 3, 2024 • edited Loading

mvayngrib commented May 3, 2024

mvayngrib left a comment

Choose a reason for hiding this comment

mvayngrib May 3, 2024

Choose a reason for hiding this comment

mvayngrib May 3, 2024

Choose a reason for hiding this comment

korthout commented May 9, 2024 • edited Loading

mvayngrib commented May 9, 2024

tasso94 commented Dec 19, 2023 •

edited

Loading

korthout left a comment •

edited

Loading

korthout commented May 3, 2024 •

edited

Loading

korthout commented May 9, 2024 •

edited

Loading