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?
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).
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 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, 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.
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.
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.