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

propose-downstream: Avoid creating divergant git branches #1724

Closed
gotmax23 opened this issue Sep 10, 2022 · 23 comments · Fixed by packit/packit-service#2478
Closed

propose-downstream: Avoid creating divergant git branches #1724

gotmax23 opened this issue Sep 10, 2022 · 23 comments · Fixed by packit/packit-service#2478
Assignees
Labels
area/fedora Related to Fedora ecosystem complexity/single-task Regular task, should be done within days. gain/low This doesn't bring that much value to users. impact/low This issue impacts only a few users. kind/feature New feature or a request for enhancement.

Comments

@gotmax23
Copy link
Contributor

Whenever possible, I like to keep my distgit branches merged. It eases maintainability and testing, and ensures that the package isn't too different between Fedora releases. I make exceptions when there are different upstream major versions or when EPEL packaging quirks would require too many conditionals.

packit propose-downstream breaks this, because it creates separate PRs with separate commits for each release, even when the branches are merged. This should be made configurable to allow using the same commit for multiple branches. I believe Pagure allows submitting PRs from the same fork branch to multiple source branches, so with this approach, packit wouldn't always need to create separate branches.

Packit could also determine when the distgit branches are perfectly merged and just do this automatically, but that can be a future improvement.

@lachmanfrantisek
Copy link
Member

Hi @gotmax23!

That's a good suggestion.

For now, you can let Packit open a single pull request and handle the other branches yourself. But I cannot find a nice way to open a second pull-request from a Packit's fork. Maybe using a remote pull-request functionality -- e.g.: https://src.fedoraproject.org/rpms/YOUR_PACKAGE_NAME/diff/remote
You can also merge the first pull-request and open other pull-requests from the merged branch.

Regarding the implementation:

  • We need to check that we don't do anything branch-specific during the process so we are able to reuse the prepared content. (Both via CLI and service.)
  • Reporting and database manipulation need to be updated.

@lachmanfrantisek lachmanfrantisek moved this from new to backlog in Packit Kanban Board Sep 12, 2022
@gotmax23
Copy link
Contributor Author

But I cannot find a nice way to open a second pull-request from a Packit's fork.

The user interface doesn't show the option, but it's possible.

See https://src.fedoraproject.org/rpms/packit/pull-request/372 and https://src.fedoraproject.org/rpms/packit/pull-request/373.

I created one with https://src.fedoraproject.org/fork/gotmax23/rpms/packit/diff/rawhide..1724_test and another with https://src.fedoraproject.org/fork/gotmax23/rpms/packit/diff/f37..1724_test

@lachmanfrantisek
Copy link
Member

Sorry, I meant the Packit's fork when I am not a Packit user. Similarly, I am unable to create a new pull-request using your fork:
Screenshot from 2022-09-12 19-45-34

@stale
Copy link

stale bot commented Nov 12, 2022

This issue has been marked as stale because it hasn't seen any
activity for the last 60 days.

Stale issues are closed after 14 days, unless the label is removed
by a maintainer or someone comments on it.

This is done in order to ensure that open issues are still relevant.

Thank you for your contribution! 🦄 🚀 🤖

(Note: issues labeled with pinned or EPIC are
never marked as stale.)

@stale stale bot added the stale Is the issue still valid? label Nov 12, 2022
@gotmax23
Copy link
Contributor Author

/me silences the user hostile stale issue bot

@stale stale bot removed the stale Is the issue still valid? label Nov 12, 2022
@lachmanfrantisek lachmanfrantisek added kind/feature New feature or a request for enhancement. area/fedora Related to Fedora ecosystem complexity/single-task Regular task, should be done within days. gain/low This doesn't bring that much value to users. impact/low This issue impacts only a few users. labels Apr 11, 2023
LecrisUT added a commit to LecrisUT/scikit-build-core that referenced this issue Apr 13, 2023
Avoid issue: packit/packit#1724

Signed-off-by: Cristian Le <[email protected]>
henryiii pushed a commit to scikit-build/scikit-build-core that referenced this issue Apr 13, 2023
Slight fix to the PR targets for Fedora. There is an
[issue](packit/packit#1724) that commits there
diverge, so I'll have to manually create PRs on branches until then.

Also, need to add `.idea` to gitignore otherwise sdist pre-commit check
complains. Maybe a similar thing will be needed for vs-code and across
the other projects?

Signed-off-by: Cristian Le <[email protected]>
@psss
Copy link

psss commented Jun 9, 2023

This would be a very nice improvement! We've kept tmt branches synced before enabling propose-downstream and it would be nice if we could return to that approach as it makes is easier to apply other changes (e.g. adjusting test config).

@LecrisUT
Copy link
Contributor

LecrisUT commented Jun 17, 2023

But I cannot find a nice way to open a second pull-request from a Packit's fork.

Maybe it's not possible in the UI, but could it be achieved in the api backend. I did make new PRs on my own fork from the same branch, but a simple workaround is to make a new branch from the original one (should be the case to allow branch specific changes) amd then make a PR against that

I think the workflow should be similar to the copr_build and how it sends all builds at once and then splits up its monitoring:

  • git pull the latest downstream
  • go from high to low on the targets least (ordered from rawhide down) and check which branches are not divergent
  • on the highest branch in each group
    • create a PR branch from downstream
    • sync the files and create a commit
  • on all other branches in the group
    • wait for the commit to finish
    • create a PR branch from the leader branch of the group
  • wait for all branches to finish and submit PRs together. This makes it easier to retry and not diverge the commits

Edge-case: Only rawhide PR has been merged and all other branches are currently divergent. This expands on step 2 above:

  • when checking each branch to create a group, if the branch is not the highest, try-catch to git merge --ff-only {leader-branch}
  • if git merge --ff-only fails, create a new group and set current branch as leader and continue with lower branch
  • when all groups are made, continue with the logic above

(Also impact is rather medium or more because it ensures we follow fedora packaging guidelines)

@lachmanfrantisek
Copy link
Member

lachmanfrantisek commented Jul 6, 2023

The issue with the implementation is that for now, the runs are quite separated and you can't easily report/rerun per group. Otherwise, it's easy. In the time of opening a PR, we can just run the command to create a PR multiple times with different attributes.

The easest (but a bit hackish) to implement might be to let user configure the set of branches that shares the same run. (This will allow us to know that sooner than from the git.)

@lbarcziova lbarcziova added the discuss discuss To be discussed within a team (usually on the so-called Architecture meeting next Thursday) label Oct 19, 2023
@majamassarini
Copy link
Member

majamassarini commented Oct 26, 2023

Do a research in general and also on using Gitlab (for CentOs Stream and probably future integration)

One example could be: do a PR just for rawhide and then use other options/tools to automate merging on other branches.
Create a documentation for helping do manually the merge on other branches and add it to the PR instructions.

@majamassarini majamassarini moved this from backlog to ready-to-refine in Packit Kanban Board Oct 26, 2023
@majamassarini majamassarini removed the discuss discuss To be discussed within a team (usually on the so-called Architecture meeting next Thursday) label Oct 26, 2023
@lbarcziova lbarcziova moved this from ready-to-refine to backlog in Packit Kanban Board Nov 23, 2023
@prestist
Copy link

prestist commented Jan 22, 2024

This would be excellent, we recently adopted packit for one of our repos and found that we could no longer fast forward, while this is workable, the result is we will no longer able to compare our branches to see any fixes at a glance. In the past we would compare branches to quickly glance and easily find what was different from f39 or f37, but now that will be a little bit more effort. We certainly want to keep using packit for the reduction in day to day steps but, we would certainly prefer to maintain the fastforwardable nature if possible.

@lbarcziova lbarcziova moved this from backlog to priority-backlog in Packit Kanban Board Mar 14, 2024
@LecrisUT
Copy link
Contributor

LecrisUT commented Jul 5, 2024

Oh, I think there is a simple tool to manage this fbrnch, particularly fbrnch merge. This can take care of figuring out how to merge from the newer branch. @juhp are there other features that could help here?

@juhp
Copy link

juhp commented Jul 8, 2024

For reference if it helps, this is the code fbrnch is using for gitMergeable

@mfocko
Copy link
Member

mfocko commented Jul 18, 2024

Notes from refinement

  • Be careful about potential rebuilds
  • Introduce new config option for fast-forwarding via PR to co-rawhide branches (on merge)
    • potential implementation: after merge to rawhide, check out the rawhide, push to Packit's fork, and open PRs (Pagure should fast-forward)

@mfocko mfocko moved this from priority-backlog to refined in Packit Kanban Board Jul 18, 2024
@lachmanfrantisek
Copy link
Member

lachmanfrantisek commented Jul 18, 2024

  • potential implementation: after merge to rawhide, check out the rawhide, push to Packit's fork, and open PRs (Pagure should fast-forward)

Thinking about this, we can even do a similar thing:

  • When configuring, allow one branch to be "the source` (~rawhide) and multiple other branches to "follow".
    • Either provide another config option (something like create_pull_request_to, follow_with_branches, merged_into, use_for_branches,... better names welcome).
    • Or, support mapping between branches. Or any other syntactical sugar in config.
  • Do the sync release as usual but check the new config once we are done with PR submitting (to rawhide) and open extra PRs to other "follow" branches from the same commit.
  • This will not break the workflow, service, database, reporting,... (The only change might be to support a list of PRs in db/dashboard instead of a single one.)

Example config:

- job: pull_from_upstream
  trigger: release
  dist_git_branches:
   - fedora-rawhide
  create_pull_request_to:
   - fedora-all

@nforro
Copy link
Member

nforro commented Jul 18, 2024

Example config:

- job: pull_from_upstream
  trigger: release
  dist_git_branches:
   - fedora-rawhide
  create_pull_request_to:
   - fedora-all

I like this, but it allows for "impossible" combinations:

- job: pull_from_upstream
  trigger: release
  dist_git_branches:
    - f40
    - epel9
  create_pull_request_to:
    - fedora-rawhide
    - epel8

What about something similar to what we support with targets in test jobs:

- job: pull_from_upstream
  trigger: release
  dist_git_branches:
    fedora-rawhide:
      open_pull_requests_for: [fedora-all]
    epel9: {}

@LecrisUT
Copy link
Contributor

Sounds good. One note on fbrnch is that it does sequentially rawhide -> f40 -> f39 (epel as well, but didn't check where it branches off), and let's say f40 has diverged, f39 will try to fast-forward to f40 instead of rawhide. Having a similar approach would be useful.

Let me understand the workflow a bit:

  • First create the PRs to dist_git_branches and wait for PRs to merge
  • After all PRs are merged, create the remaining PRs from create_pull_request_to

My thought is that in this interface the latter part could be split as a separate job like sync_branches (and be independent of pull_from_upstream/propose_to_downstream).

Another interface could be:

  • Create all PRs to dist_git_branches in sequence
  • Expose an interface /packit rebase to bring the PR branches up-to-date to the PR's branch

@majamassarini majamassarini self-assigned this Jul 22, 2024
@majamassarini majamassarini moved this from refined to in-progress in Packit Kanban Board Jul 22, 2024
@majamassarini
Copy link
Member

@nforro @lachmanfrantisek I think that as a first implementation, to keep things simple on the packit side, we don't want to check if the branch can be fast forwarded, right? Is up to the user to be sure this feature can be used, right?
I think that the @LecrisUT suggestions can be handy for an evolution of the very first implementation, WDYT?

I will look for the open_pull_requests_for key (and I will follow @nforro suggestion to have it nested below the specified dist-git-branch) and when I find it I will try to open a PR for all the listed open_pull_requests_for entries.

Thinking at the case when a branch can't be fast-forwarded, the user will have to change the configuration, removing the non fast-forwardable branch from the open_pull_requests_for list, he will have to re-trigger manually the propose-downstream or pull-from-upstream job and I would expect the opened PR for the given target will be updated with a different source branch (hopefully). Am I missing something? Or do you agree?

prestist pushed a commit to HuijingHei/coreos-installer that referenced this issue Jul 22, 2024
Additionally set propose-downstream to fedora-rawhide
until packit/packit#1724 is resolved.

Co-authored-by: Huijing Hei <[email protected]>
@travier
Copy link

travier commented Jul 23, 2024

Let me understand the workflow a bit:

  • First create the PRs to dist_git_branches and wait for PRs to merge
  • After all PRs are merged, create the remaining PRs from create_pull_request_to

I think I prefer this flow compared to the rebase option, which is not really a rebase, so would be confusing (and might lead to mistakes as the PRs would already be opened).

@LecrisUT
Copy link
Contributor

Rebasing interface might still be necessary at some point. Here's an example that I just encountered:

  • packit opened a PR to update a package
  • Fedora 41 mass rebuild happened -> rawhide and f40 started to diverge
  • Merging on rawhide would create a merge commit and on f40 it would just rebase
  • Need an interface to fast-forward if possible, or cherry-pick/rebase commits

The order where the new PRs are created only after the main PR is merged, would help a lot in this situation, but there are other situation where things can unexpectedly diverge, e.g. rebuilds due to SOVERSION change, work on CI backend, etc.

@travier
Copy link

travier commented Jul 23, 2024

Indeed. Good point.

@majamassarini
Copy link
Member

@travier @LecrisUT I think I get your points.

Unfortunately from the packit code perspective this imply more changes (the propose-downstream task can no run anymore as just one Celery task but should be somehow splitted in multiple subtasks that reacts not only to a release event but also to merge events). It can be done but requires more effort.

For this reason I would start with a simple and not at all perfect solution. Where, it is up to the user to be careful to map only fast-forwardable branches. But still, if something goes wrong (and you listed a lot of possibilities that can make things go wrong), there should be a workaround and I think that changing the configuration and retriggering the propose-downstream job should do the trick even though it is not a good user experience; I will try and test it out.

As next step I think that having the rebase command in packit can be a good and quick improvement, even though not a perfect solution yet.

As last step, we can work on splitting up the propose-downstream task and make it happen in sequential little steps.

The last two points can be follow-up cards of the very basic first implementation.

@lachmanfrantisek
Copy link
Member

@travier @LecrisUT , as Maja has suggested, your approach sounds really valid but I am worried it will take a significant time to make happen. So, unless we have a helping hand from someone outside the team, we need to start small..;) But, I am glad we have it here so we can continue on this once we are done with a basic implementation.

Regarding the first iteration, I would:

  • Not solve any rebasing, just make it clear to the user, suggest a resolution (maybe even git commands to be copy-pasted) and retrying.
  • Do everything in one run (creating to post-rawhide-merge can be left for the future).
  • Ad merging strategies and other git functionality -- would be nice not to reinvent the functionality that should be covered by a git-forge (and moving away from Pagure might actually help here).

@majamassarini could you please create a new cards for these follow-ups so it's not lost in history when we close this card?

@lachmanfrantisek
Copy link
Member

Regarding the config schema, I like the mapping style @nforro has suggested.

@majamassarini majamassarini moved this from in-progress to in-review in Packit Kanban Board Aug 28, 2024
softwarefactory-project-zuul bot added a commit that referenced this issue Sep 5, 2024
Allow dist_git_branches to be a dictionary

now, dist_git_branches can be

or a list of strings:

["rawhide", "fedora-stable"]


or a complex dictionary

{
  "rawhide": {
    "fast_forward_merge_into": ["f40"]
   },
  "fedora-stable": {}
}

Related with #1724
Should fix #2375

 update documentation

Reviewed-by: František Lachman <[email protected]>
Reviewed-by: Maja Massarini
Reviewed-by: Laura Barcziová
Reviewed-by: Nikola Forró
@github-project-automation github-project-automation bot moved this from in-review to done in Packit Kanban Board Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/fedora Related to Fedora ecosystem complexity/single-task Regular task, should be done within days. gain/low This doesn't bring that much value to users. impact/low This issue impacts only a few users. kind/feature New feature or a request for enhancement.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.