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.

Sign up for GitHub

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

Jump to bottom

Automatically open PRs to sync branches #13767

Open
wants to merge 4 commits into
base: 2.5
Choose a base branch
from
Open

Automatically open PRs to sync branches #13767

wants to merge 4 commits into from

Conversation

Holzhaus
Copy link
Member

@Holzhaus Holzhaus commented Oct 14, 2024 •
edited
Loading

acolombier
acolombier approved these changes Oct 14, 2024
View reviewed changes
Copy link
Member

@acolombier acolombier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Just some small questions, but nothing preventing the merge.

.github/workflows/sync_branches.yml
Comment on lines +5 to +6
- "2.4"
- "2.5"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is is possible to use a pattern here, like you have in the labeller or does the API expect a static branch name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fnmatch syntax (glob-like), so we could use [0123456789].[0123456789]. But it's a bit pointless since we still need to hardcode the from/to branch mapping below.

.github/workflows/sync_branches.yml
Comment on lines +14 to +16
- from_branch: "2.4"
to_branch: "2.5"
- from_branch: "2.5"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we always backport via the n-1 version? What I mean by that is, if there is a change in 2.4, would we expect 2.4 -> 2.5 PR to be reviewed and merged, then 2.5 -> main? Or should we consider 2.4 -> 2.5 and 2.4 -> main to be created in parallel?
I'm thinking, there could be case we the 2.4 -> 2.5 (or any stable versions) needs to be delayed due to issues or further work needed to support the backport, but in the meantime, there wouldn't be a need to hold it for the dev branch (main)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should almost always just go to the next branch, not open multiple PRs from the same source. This just leads to more noise and confusion.

If we ever need this, nothing is stopping us from opening a PR manually 😉

@Holzhaus Holzhaus marked this pull request as ready for review October 14, 2024 22:12
Swiftb0y
Swiftb0y approved these changes Oct 15, 2024
View reviewed changes
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you. Does it matter which branch this goes into? does still run the 2.4->2.5 job if it only lands in main?

@Holzhaus
Copy link
Member Author

Does it matter which branch this goes into? does still run the 2.4->2.5 job if it only lands in main?

IIRC it suffices to have it in the default branch (main), but I'll double check on the playground repo.

@Holzhaus
Copy link
Member Author

Or I'll just rebase onto 2.4 to be on the safe side.

@Holzhaus Holzhaus changed the base branch from main to 2.4 October 15, 2024 16:07
@Swiftb0y
Copy link
Member

Great thanks. Lets give everyone 24h to object before we merge.

@Holzhaus
Copy link
Member Author

Holzhaus commented Oct 15, 2024 •
edited
Loading

I think this is actually lacking the instruction from gitattributes (although I don't remember making that configuration change before). Is this really needed? If so, I need to change this workflow and create a dedicated branch instead.

@acolombier
Copy link
Member

Is this ready to be merged? I was about to cherry-pick #13913 into 2.5 and main but I think it would be a great test case for this feature!

@Holzhaus
Copy link
Member Author

Holzhaus commented Dec 5, 2024

I think this is not usable due to the translations mess. See #13767 (comment).

@Holzhaus Holzhaus force-pushed the sync-branches branch 4 times, most recently from 7577eb0 to 0fcebf0 Compare January 12, 2025 12:36
@daschuer
Copy link
Member

In the past we the merge commit was always the auto generated message:
Merge remote-tracking branch 'upstream/2.5' Can we keep it?

In your Demo branch it is now:

Merge pull request https://github.com/mixxxdj/playground/pull/10 from mixxxdj/2.5
Merge changes from `2.5` into `main`

It is unfortunately less obvious that this is no functional change.

How shall it work finally? Collect all merged PRs until one of us presses merge?
Or merge every single PR of a stable branch automatically?

Current solution:

  • Wait for a contributor who merges and test locally
  • In case of conflicts, wait for an experienced contributor or
  • Do a preliminary merge as a PR, if this is successful confirmed merge manually

How about just model this:

  • Add a workflow that updates a single PR whenever a PR was merged in a version branch.
  • Add a text command to create a direct merge commit like we have before when all checks are green

@daschuer daschuer changed the base branch from 2.4 to 2.5 January 25, 2025 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoergAtGithub JoergAtGithub JoergAtGithub approved these changes

@Swiftb0y Swiftb0y Swiftb0y approved these changes

@acolombier acolombier acolombier approved these changes

Assignees
No one assigned
Labels
code quality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Holzhaus @Swiftb0y @acolombier @daschuer @JoergAtGithub