On 8/24/21 3:55 PM, Beckerle, Mike wrote: > >>> Workflows will not run on pull_request activity if the pull request >> has a merge conflict. The merge conflict must be resolved first. > >> I think users do just need to rebase and fix the conflicts. > > But that means the comment history of the PR will be lost, as one will have > to force-push to the PR branch. > > I am thinking we could fix our workflow to address this. I am not sure it is > worth it though. > > When creating a bug branch, also create a "point of departure" branch for the > master branch point of departure. > E.g, to fix bug 5555 create daf-5555-master from master, and then > daf-5555-bug from daf-5555-master. > > PRs are created for merging back to this departure branch (daf-5555-master) > and so will never have conflicts. So CI tests will always run. > > Feedback on the changes would then be independent of any conflicts with > ongoing things merging to the master branch. Once all changes are complete, > the changes can be squashed and merged back to the point-of-departure branch > (daf-5555-master), and that branch can subsequently be rebased onto the > master branch, which would involve fixing all conflicts. > > This is yet another step, and is only needed if a PR will be open and > outstanding long enough to require it, but that's something one can't > necessarily anticipate will be needed or not, so you'd have to just always > work this way. > > I may try doing my next changes in this model.
One downside of this is that only committers can create new branches. So non-committers would have to ask a committer to create a new branch that they can merge the PR into, make sure they select the correct branch, etc. All this just to open a PR seems like too much of a barrier to entry. And then there's more work to merge a PR. We have to accept the PR into the bug-master branch, then test that before actually merging into master. We'd probably want that to be done via a PR as well to get the CI checks. So two PR's are needed for a single change, and they aren't really linked together in any way. Also, looking the PR where this came up: https://github.com/apache/daffodil/pull/601 all the previous comments do exist in the Conversation tab, some just show as outdated. So things aren't completely lost. And my guess is those "outdated" onces were on code that changed, so maybe the comments aren't even relevant for the latest changes. Maybe GitHub fixed the issue where force pushes lost comments, and this is a non-issue? I'd also argue the problem of conflicts doesn't come up enough to warrant extra complexity in the workflow. PR conflicts are pretty rare. And keeping PR small avoids this, as well as makes for easier chunks to review.
