OK, so Travis does run. If it fails, will it be visible? Usually we notice because GitHub puts a red X by the PR. At that point, the PR is merged, so it won’t show up there. Will it show up in the list of commits for master? If so, I don’t know how often people check that.
I guess if the maintainer does a rebase at all, there is a chance for the PR code and the new master code to clash. There is nothing saying that after that first rebase, a new PR is merged that changes half the files. This could easily conflict with the PR we want to merge. So we’re trying to walk a tightrope between having the dev rebase constantly to travis failing on master. If travis fails on master, it will fail for all other PRs. This will halt merges into master until @subhasisb fixes it (he owns the repo and is the only person who can force push to master).
I guess that’s the question that needs an answer. How much risk are we willing to take? One one side we have the ease of use and gain of productivity by not requiring constant rebasing (some 12 hrs apart). On the other side we have the risk of the productivity loss of our project being down until @subhasisb has the time to fix it.
With all that said, I think I am now agreeing with @subhasisb. It needs to be a judgement call on the side of the maintainer. If we have a small PR like a 3 line change to a test, the maintainer can merge it directly. If it is a larger PR, the maintainer can have the developer rebase.
@agrawalravi90, That’s why I mentioned the problem statement IS the solution. However I do not know if our CI tools can be configured for such work flow.
I doubt if this is turned on for our master as I have not seen ‘double builds’ after PR merge. see ‘Double builds’ on pull requests # which is also dicussed here and here
I think what we need is a Two Step PR merge flow:
Maintainer hits the “merge pull request” button on the ready PR.
This should NOT mark the PR “merged” yet, instead it should be marked something like “merge in progress”. At this point all other PR’s should be made ineligible for merge, basically applying a big lock on the master and also the merging PR (so as to prevent dev from further commits while merge in progress).
this trigger’s the CI builds on the master + merging feature in the PR
If any CI check fails, the merging PR is reverted to “open”. Also unlocks the merging PR and master for other PR merges
If all CI checks pass, the merging PR is marked “merged”. Also unlocks the master for other PR merges.
I remember such a work flow in my previous project working well. This will automatically serialize the contention on master from multiple maintainers and avoid unnecessary “rebases” of PRs and its CI time.
I think similar requirement is being raised in this GitHub issue
I feel we are digressing from the ‘policy’ nature of this thread. I don’t think we need to discuss specific mechanics. Things like changing our CI system may not be trivial.
As a contributor, I would only be super happy to not have to squash/rebase multiple times. I think it is really up to the maintainers to figure out how far they can support that quest.
I suggest, as contributors, we provide feedback on items 1-4 on this thread
I request the maintainer team to work out the specifics of item #5 and provide an update (assuming the contributors want #5, if possible, or as much as possible)
As all the comments so far are very positive in favor of the proposals, it looks like there is general consensus to move forward (with the specific implementation details to be worked out later by the maintainers as per @subhasisb suggestion). Great!
Since the suggested changes are relatively major, let’s leave this topic open for discussion for another week to allow the broader community plenty of time to weigh in. To that end, here’s a poll that will be open for the next week (closing on Feb 16th) – your input encouraged!
Are you in favor of the proposed process changes?
Yes, make it so! The proposed changes at the top of this thread look good.
No, some or all of the proposals need more thought and discussion.
0voters
If you vote “No”, please also consider adding to this thread below with your specific comments.
Great – as there is general consensus, we will move forward and make the proposed changes over the next few weeks. Thanks to all who provided their input!
Update – #1 is Complete – Move from Jira to Github for issue tracking
The Github issue tracking is now turned on (https://github.com/pbspro/pbspro/issues). Please use Github for raising issues moving forward. Jira will continue to also work for “a while”, but will eventually be de-commissioned.
Update for #2: Migrate from Confluence to Github pages for Design documents
After reviewing how Github pages and wikis work, we have concluded that Confluence is the better choice. So, we are sticking with Confluence for Design Docs.
Update – the rest of the proposal has been implemented:
3. Remove the requirement to have commit signatures – Signed commits are now optional.
4. Replace the checklist in the pull request (PR) with a pointer to the guidelines – Done.
5. Have maintainers squash, rebase and merge after code reviews pass (eliminating a sometimes iterative hassle for contributors) – Maintainers will now handle merges after reviews.