[rudder-dev] Improving Pull Request merges and up-merging
Jonathan Clarke
jonathan.clarke at normation.com
Wed Jun 15 10:12:14 CEST 2016
Hi Janos,
Thanks for sharing this feedback and the CI idea. We are in fact already
using that pattern, with automated tests built in to several repos (see
rudder-techniques/scripts/check-techniques.sh and
ncf/tests/acceptance/testall for example) that are run on each change.
In the future we intend to extend our CI system to run these on
appropriate Pull Requests for earlier test checking (and to avoid
committing something that doesn't pass tests).
However, the use case about merging Pull Requests turned out to not fit
into this pattern well. It basically meant a lot of work in Jenkins to
"cheat" on which branches were used. In the end a standalone Python
script, that Benoît wrote about on this list yesterday, is just about
100 lines, so simple and easy to maintain.
The future is more automated testing and automatic tests on Pull
Requests, for sure :)
Jon
On 04/29/2016 08:12 AM, Mattyasovszky János wrote:
> Hi All,
>
> First of all, thank you for investing work in making the merge
> workflow more robust, I belive we all will benefit from this.
>
> What you are describing however, looks very much like something that
> exist in various forms, and is called CI system, which for example
> Jenkins or gitlab's CI Runner are, not sure if it's worth to re-invent
> the wheel, there are a lot of github plugins which are designed to
> interface with gitlab and provide a nice way to interface with an
> automated build system, like:
> https://github.com/jenkinsci/ghprb-plugin/blob/master/README.md
> https://gitlab.com/gitlab-org/gitlab-ci-multi-runner
>
> This OTOH would require to write automated tests for the bugs /
> issues, which can be tested for later re-occurrence. However, once the
> tests are there, you could basically run automated tests on each
> commit / PR, so you can ensure the code quality is on a stable level,
> and you don't have to do so hard work on testing before releasing.
>
> Well, yes, it requires compute resources to run through all tests, but
> fortunately Jenkins is a widely used to have plenty of plugins which
> can be used to add different methods of testing, so modelling any
> existing testing workflow should be possible.
>
> Regards,
> Janos
>
> Jonathan Clarke <jonathan.clarke at normation.com
> <mailto:jonathan.clarke at normation.com>> ezt írta (időpont: 2016. ápr.
> 28., Cs, 16:28):
>
> Hi,
>
> Let me start this discussion with a bit of context:
>
> 1) Currently, all changes to Rudder repos are done via GitHub Pull
> Requests. GitHub is a great tool for this kind of collaboration,
> and we see no reason to change from it, currently. It allows for
> easy discussion about the proposed change, directly in the code.
> Once the discussion has ended in a consensus, the proposed code
> change is merged directly from the GitHub web interface into the
> branch it's targeted for.
>
> 2) Currently, all Rudder repos have one branch per major version
> (2.11, 3.0, 3.1, etc). All bug fixes are targeted to the oldest
> version in which the bug is present, then a semi-automatic process
> "up-merges" the fix from the older branches to the newer ones,
> ending up in master. This process is automatic when the merge is
> "fast-forward" (the change can be applied to the same "before"
> code in the upper branches), and is run automatically 3 times a
> day. However, when the up-merge generates a conflict (the "before"
> code has changed between branches), this requires a manual
> operation to up-merge the fix, outside of GitHub's web interface,
> directly using git CLI tools.
>
> This situation causes some issues, currently:
>
> 1. Often, several changes are merged into a repo before the
> manual operation to up-merge is done. This makes this manual
> operation even harder, because whoever does it needs to
> understand *all* the changes going in, and think about the
> consequences of all of them at the same time. This causes
> merges to sometimes not be done for days, until the right
> group of people is available. Sometimes, it can end up in a
> mistake (wrong line included/removed, merge markers left in,
> etc...)
> 2. Often, the person reaction to automatic merge errors is a
> different one from whoever wrote or merged the PR. This means
> they don't know the code, and this will both make the manual
> up-merge harder and more error-prone.
>
>
> Basically, the current approach is error-prone (there is a
> significant number of tickets opened to fix merge errors in
> Redmine), and causes unnecessary difficulty, thus delays. We've
> been looking for a way around this for a while, and in a meeting
> yesterday, we just came up with one.
>
> The idea:
>
> 1. Keep PR submission and review as they currently, but don't
> merge via GitHub's web interface.
> 2. Once a consensus has been reached, instead of clicking on the
> "Merge" button in GitHub's web interface, the reviewer adds a
> tag to the PR that says something like "Ready for merge".
> 3. At this stage, an automatic script will check out the PR, and
> merge it into the target branch, and test all up-merges to
> higher branches.
> 1. If they are all "fast-forward" merges, the script will do
> the merges, and commit the result. Therefore, a change
> will simultaneously appear in all branches, the PR will be
> closed automatically (GitHub detects this). In this simple
> case, we're done, and the merge was almost immediate.
> 2. If the up-merges require manual intervention, the script
> will comment on the PR. The PR author is then responsible
> for merging his own PR (alternatively, this can be the
> reviewer too, if the author does not have commit access).
> An interactive script will be developed (likely an
> extension of rudder-dev) that follows the same workflow as
> described in step 3.1 above, but letting the user
> interactively fix the conflicts. Then, the result will be
> commited simultaneously to all branches, the PR
> automatically closed.
>
>
> The main benefit of this approach is clear: only authors and
> reviewers of the code will be doing the up-merges, thus reducing
> how error-prone these merges currently are once, and also only one
> PR will be up-merged at a time, thus reducing how error-prone
> these merges are again. Extra benefits include the option to
> customise the workflow after a PR is merged, including things like
> automatically updating the Redmine ticket, listing any
> contributors (commit authors) in the next changelog, etc...
>
> There are some drawbacks, in particular that this will cause us to
> move away from the standard GitHub merging workflow. This is in
> fact one of the options that GitHub documents (if you read
> carefully next to the "Merge" button there is some text explaning
> you can do this on the command line). Also, this won't affect
> *contributors* workflow in GitHub, only committers, so targets a
> reduced population.
>
> Benoit is working on a prototype, I believe.
>
> What are your thoughts on this? Good idea, go, or bad idea, please
> don't do that? Any questions?
>
> Jon
> --
> ------------------------------------------------------------------------
> *Jonathan CLARKE*
> /Co-founder & Chief Product Officer/
> Normation <http://www.normation.com>
> ------------------------------------------------------------------------
> *87 rue de Turbigo, 75003 Paris, France*
> Telephone: +33 (0)1 83 62 41 24
> Mobile: +33 (0)6 99 60 03 10
> ------------------------------------------------------------------------
>
> _______________________________________________
> rudder-dev mailing list
> rudder-dev at lists.rudder-project.org
> <mailto:rudder-dev at lists.rudder-project.org>
> http://www.rudder-project.org/mailman/listinfo/rudder-dev
>
>
>
> _______________________________________________
> rudder-dev mailing list
> rudder-dev at lists.rudder-project.org
> http://www.rudder-project.org/mailman/listinfo/rudder-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.rudder-project.org/pipermail/rudder-dev/attachments/20160615/659b9848/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: image/gif
Size: 1036 bytes
Desc: not available
URL: <http://www.rudder-project.org/pipermail/rudder-dev/attachments/20160615/659b9848/attachment-0001.gif>
More information about the rudder-dev
mailing list