You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by "Tutkowski, Mike" <Mi...@netapp.com> on 2018/05/01 18:53:41 UTC
Re: [DISCUSS] new way of github working
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" <ma...@renemoser.net> 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é
Re: [DISCUSS] new way of github working
Posted by Rohit Yadav <ro...@shapeblue.com>.
My preference is to not squash commits when a PR is created, I do prefer squash merge of PR. Also, instead of a rule I would prefer this to be a general guidelines, as there may be cases where it may be valid to have few commits in which case I will prefer doing a rebase+merge (via github or otherwise). As an example, if a PR is worked on by several people (say it's a big change, or someone carries forward an old PR whose primary author is not responding), I can prefer to have commits melded by author (to give credit to authors involved) or component (say on test, scripts/infra, java packages,).
I think during course of PR review, not squashing commits helps reviewers to track what got changed from the initial PR.
Merging of PR with an explicit merge commit is something I don't like as it messes up the git history.
- Rohit
<https://cloudstack.apache.org>
________________________________
From: Rafael Weingärtner <ra...@gmail.com>
Sent: Wednesday, May 2, 2018 4:53:41 PM
To: dev
Subject: Re: [DISCUSS] new way of github working
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 <Mi...@netapp.com>
wrote:
> 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" <ma...@renemoser.net> 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
rohit.yadav@shapeblue.com
www.shapeblue.com
53 Chandos Place, Covent Garden, London WC2N 4HSUK
@shapeblue
Re: [DISCUSS] new way of github working
Posted by Rafael Weingärtner <ra...@gmail.com>.
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 <Mi...@netapp.com>
wrote:
> 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" <ma...@renemoser.net> 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