cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rafael Weingärtner <>
Subject Re: [DISCUSS] new way of github working
Date Wed, 02 May 2018 11:23:41 GMT
I think we are on the same page here. There is only one thing that is
different from what we are doing in PRs.

> Before you create a PR, squash all applicable commits to make it more
> readable for reviewers

I would suggest not squashing everything before creating the PR. I mean, do
the following when coding:

   - disable your auto formatter (in Eclipse you can just disable the save
   actions) if you are going to change a class that might need formatting
   - do the changes
   - create a commit for the main changes of the PR
   - enable the auto formatter and formatting the change files
   - create a commit for formatting and cleanup changes
   - then create the PR

Separating formatting/cleanups commits makes it easier for reviewers to
evaluate changes.

On Tue, May 1, 2018 at 3:53 PM, Tutkowski, Mike <>

> Hi everyone,
> We had a good conversation going here. Maybe we can continue it, get some
> level of reasonable consensus, and implement it (if, in fact, the consensus
> is a change from what we currently have).
> My suggested approach is the following:
> Before you create a PR, squash all applicable commits to make it more
> readable for reviewers. Once reviews start coming in and you start making
> changes, push new commits on top of the prior ones (do not squash at this
> point). This will make it easier for reviewers to confirm that you and they
> are on the same page with regards to what was changed. When you need to
> draw in changes from the base branch, rebase your commits on top of it.
> When the PR is given a LGTM by 2+ reviewers and passed the necessary
> regression tests, it should be squashed and then merged. I see the
> evolution of commits during the life of the PR as a temporary sandbox of
> history that is no longer required once the PR has been completed.
> I think that process should cover the vast majority of our PRs.
> There are usually some exceptions to the rule, however. When this happens,
> discuss your situation with the reviewers and bring any concerns to the
> mailing list before deviating from the standard process.
> Thoughts?
> Mike
> On 1/15/18, 1:47 PM, "Rene Moser" <> wrote:
>     On 01/15/2018 09:06 PM, Rafael Weingärtner wrote:
>     > Daan,
>     >
>     > Now that master is open for merges again, can we get a feedback
> here? It
>     > might be interesting to find a consensus and a standardize way of
> working
>     > for everybody before we start merging things in master again …
>     +1 to allow merge commits on master branch to keep history of a series
>     of patches when they help to understand the change.
>     René

Rafael Weingärtner

  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message