This is a proposal (based on community feedback) to make the following changes to the contribution process to make it simpler and easier:
1. Move from Jira to Github for issue tracking 2. Migrate from Confluence to Github pages for Design documents
3. Remove the requirement to have commit signatures (signed commits)
4. Replace the checklist in the pull request (PR) with a pointer to the guidelines
5. Have maintainers squash, rebase and merge after code reviews pass (eliminating a sometimes iterative hassle for contributors)
Please use this Topic to provide your feedback (positive or negative).
Once community consensus is reached, we will “make it so”.
Hey @billnitzberg Just wanted one clarification on 5th point, when maintainers do squash and rebase before merge and if there are lots of conflict (mostly this will happens in big changes) then, is it ok to ask contributors to rebase and resolve conflict?
Good question @hirenvadalia – I think #5 should read:
Allow maintainers to squash, rebase and merge after code reviews pass (eliminating a sometimes iterative hassle for contributors).
I feel it is always the case that the maintainers and contributors will collaborate on the best approach, especially for major contributions and/or to resolve conflicts. The intent was to eliminate the requirement on contributors, especially for “easy” cases.
This is more of a logistical question, but how will we migrate to GitHub for issue tracking and design docs? Will they exist in multiple places for some period of time? Don’t get me wrong, I’m all for the move. Just wondering how we plan to execute it.
Have maintainers squash, rebase and merge after code reviews pass (eliminating a sometimes iterative hassle for contributors)
Just wanted to mention a complication with this: maintainers will have to be careful not to turn on git auto-sign, otherwise when they squash the developer’s commits, the commit will get the maintainer’s signature, and github will give the maintainer credit for the work along with the original author (which some people might not like? not sure, but wanted to mention it just in case).
Good point. But I think most of the merging will be done with the “Merge pull request” button near the bottom of the PR as opposed to the command line. It gives three options:
Create a merge commit
Squash and merge (currently disabled for the PBS Pro repo)
Rebase and merge
I suggest we enable the “Squash and merge” setting so that maintainers can use it in the future.
Ah ok, we’ve always merged via command line so I hadn’t even considered taking Github’s help. I think doing a merge via Github adds Github’s own signature to the commit. Maintainers should still turn off auto-signing just in case, especially now that we are proposing to not require signed commits.
I suggest we save the logistics for later, after we reach consensus on the plan. I’m confident we can come up with a good transition plan for whatever changes are decided.
I have a minor concern with #5. We ask that developers not rebase during code reviews. This means that there could be quite a bit of code that is merged in during the final rebase. If the maintainer does the squash and rebase, then neither the smoke tests nor their automated tests will be rerun. Maybe we should have the dev do the initial squash and rebase. Travis will run the smoke tests. They can run their automated tests. If another rebase is required after the original one, the maintainer can be in charge of that.
Bhroam
Bhroam, i agree, I think especially for large changes it should be to okay to ask the developer to squash/rebase to bring it closer to the branch to merge to. However, smaller contribution will benefit from the developer not having to be on his feet to merge quick enough, to avoid need for further rebases.
Yes @mkaro, that is exactly what i was thinking. Makes life of the maintainer easy too and we won’t have to go to the command line to avoid a merge commit
@subhasisb
I still think we should have the dev do the squash and rebase once. I don’t think this is much of an imposition. The dev can rerun their automated tests on larger PRs, but at least we want Travis to run the smoke tests one last time after the rebase.
Currently, the real slow down is people having to get in line to rebase and merge. This gets really bad if the people merging are on the other side of the world. I’m saying we get rid of this. When the PR is ready for merge, the dev squashes and rebases. After that, it’s up to the maintainer to rebase again. This means a dev in India won’t have to wait for a dev in the US to merge if something is merged between when they rebased and when the maintainer gets to their PR. If it is out of date, the maintainer will rebase themselves and merge.
@bhroam i think i agree to that overall. For the most part, dev would anyway squash/rebase after a “sign off” - which will allow running travis. And any further rebases can be taken up by maintainers.
However, if the contribution is really simple/small and has passed Travis each time a small commit was added, a maintainer might be totally satisfied and do the squash and merge by him/herself.
(perhaps we can additionally find out a way to trigger travis, just to be sure, in this case too). This is probably a very small percentage, so should be no issue - besides, it would be upto the maintainer to take a call in such a case…
If the maintainer squashes and does a rebase via the command line, I’m pretty sure that they cannot merge the PR without going through Travis, i.e - they will have to force push to the developer’s branch, let Travis and other checks pass, and only then the commit can go through (otherwise git push will fail). I’m not sure if this will happen when merging via Github directly though.
I know we can trigger Travis manually as well. (we will have to figure out appveyor, but should be possible). I feel we should leave the “how” aside for now…
I agree, #5 deserves a separate thread of its own. I do have 5-6 points to say on it, but I’m withholding as to not hijack the main purpose of this thread.
I also feel some of the mechanics can influence the policy.
Thanks for sending a pointer to that Shrini, it means that the thing that @bhroam was worried about might not be an issue as Travis will run on the squashed commit when a maintainer does “squash and merge” from Github. So, we actually do NOT want to do the hack that they suggested in the stack overflow solution as we want to run Travis on the squashed commit automatically one last time before the merge happens.