Better manage stacked PRs

[jbaudoux]

Currently, stacked PR are managed by asking the contributor to add a separate "do not merge" commit that lists the dependencies in the test-requirements.txt. Once the dependency is merged, we need to ask again the contributor to modify the PR to drop the change to the test-requirements.txt; it's a back and forth process implicating multiple parties.

My proposal would be to not require to list open PRs in the test-requirements.txt but rely on the PR description "Depends on" keyword. The advantages I see:

• when opening a PR, the contributor has to list the dependencies if he wants the tests to succeed. This gives good overview of the status on both PRs. As PSC, I don't have to edit the description to add them myself when some just rely on the test-requirements.txt
• if the PR is approved, we don't need the contributor to modify the PR, and maybe rebase the code and force-push. Github could rerun the tests

On the technical aspect, this means modifying the test.yml jobs:

• unreleased-deps : add a step to parse the PR description for "Depends on" to mark that step as red. We can rely on https://github.com/gregsdennis/dependencies-action
• test : append the modules modified by open PRs in the test-requirements.txt after the checkout and before the installation of the modules

What is your opinion ? What are your remarks ?

[legalsylvain]

That looks great ! Do you think it's complicated to do ?

[pedrobaeza]

The problems I see are:

• People is not going to put the dependencies properly. If they don't put correctly the commit message, putting the dependencies in a proper format...
• If the description is changed, there's no GH actions re-triggering, so a manual git operation should be done to re-trigger them.

Anyway, the syntax of the comment will be easier than the current one. Is there any workaround for the second problem?

[jbaudoux]

• If the description is changed, there's no GH actions re-triggering, so a manual git operation should be done to re-trigger them.

@pedrobaeza I think this should do the trick:

on:
  pull_request_target: 
    types: [opened, edited, reopened]

https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#pull_request_target

[pedrobaeza]

I don't think so. I think that "edited" part means the top edition (base branch changed - and maybe PR title? -):

If changing the PR title serves, it can be tricky, but it's an option.

[pedrobaeza]

But there's also a risk: to trigger a lot of checks when people put a garbage PR title and someone corrects it.

[jbaudoux]

Indeed, from what I understand, edited is triggered only when you change the target base. Using pull_request would trigger when changing the title or description but that won't be a good idea. Maybe best would be to close and reopen the PR if you had to change the Depends on in the description

[pedrobaeza]

OK, if that works, it can be an option. Don't remember if doing that, GH actions are retriggered, or they reuse last results.

[simahawk]

Thanks for the proposal @jbaudoux .

I would do it the other way around:

• the only thing you have to fill in is test-requirements.txt
• PR description is update accordingly
• if a PR is removed the PR in linked in the description is flagged as done

IMO this would be better because:

• less complicated
• keeps the flow pythonic (good for newcomers used to advanced test-req)
• less error prone
• does not require any trick in the CI
• does not hide to the developer that that's the way to test a module that depends on a pending PR

Last but not least we could improve the bot to remove the deps on merge and on rebase, or add a specific command to cleanup deps w/o having to ask to the author.

[jbaudoux]

@simahawk That was my initial idea but I saw multiple drawbacks and changed my mind.

Here are the cons:

• what is merged is not exactly what was pushed
• if you depend on multiple PRs, then once one of the PR is merged, you cannot rerun the CI
• as you need to modify what will be merged, you modify a commit or drop an entire commit; if it fails, the PR will remain green and you need to dig in the log to see why; you need to task the dev to rebase
• if one of the dependency is taken over by someone else in a new PR, you cannot modify the dependency without asking a dev to change the test-requirement.txt
• you need to teach to use that test-requirement.txt and how to write the dependency properly, the syntax is not straight forward
• when you git-aggregate multiple PRs with dependency, you need to fix conflicts with the test-requirement.txt
• I feel that it is easier to generate extra lines in the test-requirement.txt for the ci, than removing them at ocabot merge (i.e. altering the right commit)

I don't think the bot can modify the branch that is on another remote. So it cannot rebase and cannot drop the dep in test-requirement.txt but I may be wrong...

[simahawk]

• what is merged is not exactly what was pushed

What do you mean?

* if you depend on multiple PRs, then once one of the PR is merged, you cannot rerun the CI

What do you mean?

* as you need to modify what will be merged, you modify a commit or drop an entire commit; if it fails, the PR will remain green and you need to dig in the log to see why; you need to task the dev to rebase

It is already the case today: you can issue a merge command and the merge branch fails -> PR green, merge branch red -> you've got to check the merge branch build. I don't see any issue w/ this.

* if one of the dependency is taken over by someone else in a new PR, you cannot modify the dependency without asking a dev to change the `test-requirement.txt`

Managing it via description won't solve this. In any case, IMO is not ideal (if not dangerous) to replace a dependency w/o direct action by the authors of the PR.

* you need to teach to use that `test-requirement.txt` and how to write the dependency properly, the syntax is not straight forward

It's python standard. You can find docs on how to do it everywhere, docs that you don't have to maintain ;) Plus, from v17 is a bit more clear as it does not required setup/.

* when you git-aggregate multiple PRs with dependency, you need to fix conflicts with the `test-requirement.txt`

Nope, there's a trick to select only what you want (check our projs :wink: ).

Eg:

shell_command_after:
    - curl -sSL https://github.com/OCA/$repo/pull/$nr.patch | git am -3 --keep-non-patch --exclude '*requirements.txt'

* I feel that it is easier to generate extra lines in the `test-requirement.txt` for the ci, than removing them at ocabot merge (i.e. altering the right commit)

I feel that it's easier to delete a commit that is known to do only one thing. rather then editing a file :wink:

I don't think the bot can modify the branch that is on another remote. So it cannot rebase and cannot drop the dep in test-requirement.txt but I may be wrong...

That's already a problem today. We need to address this somehow. We could:

• push to our own repo
• merge
• delete the branch

@legalsylvain @sbidoul I guess you faced the same problem when working around ocabot rebase. Do you have any insight on possible solutions?

[jbaudoux]

• what is merged is not exactly what was pushed

What do you mean?

That you need to modify a commit and so you modify the commit sha. Or you need to drop a commit from the PR, so you don't really merge the PR

• if you depend on multiple PRs, then once one of the PR is merged, you cannot rerun the CI

What do you mean?

You need to ask the contributor to drop one of the line in the test-requirement.txt and repush while you could just rerun the ci and let the ci install the open dependency on top of the latest base

• as you need to modify what will be merged, you modify a commit or drop an entire commit; if it fails, the PR will remain green and you need to dig in the log to see why; you need to task the dev to rebase

It is already the case today: you can issue a merge command and the merge branch fails -> PR green, merge branch red -> you've got to check the merge branch build. I don't see any issue w/ this.

It is failing but still green. I don't find it right

• if one of the dependency is taken over by someone else in a new PR, you cannot modify the dependency without asking a dev to change the test-requirement.txt

Managing it via description won't solve this. In any case, IMO is not ideal (if not dangerous) to replace a dependency w/o direct action by the authors of the PR.

Updating the description is easy. The ci will re-run with the new dependency. The contributor or the PSC can do it. In you case, only the contributor can do it. This has no value as you still ask the contributor to drop it before merging.

• you need to teach to use that test-requirement.txt and how to write the dependency properly, the syntax is not straight forward

It's python standard. You can find docs on how to do it everywhere, docs that you don't have to maintain ;) Plus, from v17 is a bit more clear as it does not required setup/.

It is still a hack for the ci to run properly. Otherwise, this would be in requirement.txt and not in `test-requirement.txt as it is a dependency and not a test dependency. But still it is a temporary hack you need to learn when you make PRs on the OCA. While you may or may not need when working on a project.

• when you git-aggregate multiple PRs with dependency, you need to fix conflicts with the test-requirement.txt

Nope, there's a trick to select only what you want (check our projs 😉 ).

I know... but again a trick to learn...

I'm more in favor of a solution that can lower the complexity. But this is just my opinion and happy to get feedback.

[sbidoul]

hack for the ci to run properly

The ci and runboat as well

[hparfr]

https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-suggestions-to-update-pull-request-branches

It do not work cross-repo, but it works like I understand the documentation, it may facilitate the rebasing from the github UI.

[simahawk]

It is still a hack for the ci to run properly.

It's not an hack. It's they you install python packages.

That you need to modify a commit and so you modify the commit sha. Or you need to drop a commit from the PR, so you don't really merge the PR

This is not a problem. The contrary. If you rely on the description, you have no unique identifier for the status of that PR.

Updating the description is easy. The ci will re-run with the new dependency. The contributor or the PSC can do it. In you case, only the contributor can do it. This has no value as you still ask the contributor to drop it before merging.

Is not "my case". It's the current situation :) I'd rather spend time finding a way to be able to amend a PR (remember: we need this also to ease rebases by maintainers) rather then spend time in a non standard way to install pending merges.

My POV of course :)