You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by "Ju@N" <ju...@gmail.com> on 2018/05/22 09:41:48 UTC
Reviews and Conflicts in Pull Requests
Hello geode devs,
The Geode Wiki has a lot of useful information, not only about the
usage/internal architecture of Geode, but also explanations and guidelines
about how Code Contributions [1] should be made. However, some common cases
are not explained in detail and, as such, contributors (and also reviewers)
might have a hard time trying to deal with these situations in an
*standardize* manner. In particular, I'm referring to the following two
scenarios, which can happen for every single pull request:
- *Reviewers request a change: *do contributors have to make the changes
and just add another commit to the original pull request?. Do
contributors have to make changes and squash everything in one single
commit (executing a *push --force *afterwards)?. Do contributors have to
close the pull request and open a new one with everything squashed?.
- *Pull Request looks good but, after a while without being merged,
somebody else makes a change in develop, causing the original pull request
to become conflictive with the current develop branch: *should the
committer merging the pull request solve these conflicts as well?. Do
contributors have to solve the conflict locally?, if so, do they have to
add a new commit to the pull request or squash everything in one single
commit (executing a *push --force *afterwards)?.
It would be good if we could add these details to the Wiki so everyone
knows how to proceed whenever they hit this situations (git commands will
also be helpful). Would anyone be able to add this to the Wiki?, or reply
to this thread so the approved/recommended process is documented somewhere?.
Cheers.
[1]: https://cwiki.apache.org/confluence/display/GEODE/Code+contributions
--
Ju@N
Re: Reviews and Conflicts in Pull Requests
Posted by Juan José Ramos <jr...@pivotal.io>.
Hello Bruce,
Sounds good to me, thanks for the quick reply!.
Best regards.
On Tue, May 22, 2018 at 4:44 PM Bruce Schuchardt <bs...@apache.org>
wrote:
> I don't think the "single commit" notion is even a good one for an
> original PR. I've discussed this with other people and we think it's
> okay for the PR to have multiple commits if they serve different
> purposes such as renaming variables or restructuring code before
> attacking a problem. Likewise it's okay to address review comments in
> an additional commit. The PR template just addresses the initial PR
> being a single commit anyway.
>
> That being said, if the changes required by reviewers are so substantial
> that the PR becomes difficult to review it should be pulled and a new PR
> should be created. Otherwise who's going to bother with it?
>
> Regarding conflicts, that's between the contributor and the committer.
> A PR that picks up conflicts due to other peoples' work isn't invalid.
> Most conflicts are easy to resolve but if it's too gnarly would the
> contributor want someone else to do the work and maybe mess up?
>
>
> On 5/22/18 2:41 AM, Ju@N wrote:
> > Hello geode devs,
> >
> > The Geode Wiki has a lot of useful information, not only about the
> > usage/internal architecture of Geode, but also explanations and
> guidelines
> > about how Code Contributions [1] should be made. However, some common
> cases
> > are not explained in detail and, as such, contributors (and also
> reviewers)
> > might have a hard time trying to deal with these situations in an
> > *standardize* manner. In particular, I'm referring to the following two
> > scenarios, which can happen for every single pull request:
> >
> > - *Reviewers request a change: *do contributors have to make the
> changes
> > and just add another commit to the original pull request?. Do
> > contributors have to make changes and squash everything in one single
> > commit (executing a *push --force *afterwards)?. Do contributors
> have to
> > close the pull request and open a new one with everything squashed?.
> >
> >
> > - *Pull Request looks good but, after a while without being merged,
> > somebody else makes a change in develop, causing the original pull
> request
> > to become conflictive with the current develop branch: *should the
> > committer merging the pull request solve these conflicts as well?. Do
> > contributors have to solve the conflict locally?, if so, do they
> have to
> > add a new commit to the pull request or squash everything in one
> single
> > commit (executing a *push --force *afterwards)?.
> >
> > It would be good if we could add these details to the Wiki so everyone
> > knows how to proceed whenever they hit this situations (git commands will
> > also be helpful). Would anyone be able to add this to the Wiki?, or reply
> > to this thread so the approved/recommended process is documented
> somewhere?.
> > Cheers.
> >
> > [1]:
> https://cwiki.apache.org/confluence/display/GEODE/Code+contributions
> >
>
>
--
Juan José Ramos Cassella
Senior Technical Support Engineer
Email: jramos@pivotal.io
Office#: +353 21 4238611
Mobile#: +353 87 2074066
After Hours Contact#: +1 877 477 2269
Office Hours: Mon - Thu 08:30 - 17:00 GMT. Fri 08:30 - 16:00 GMT
How to upload artifacts:
https://support.pivotal.io/hc/en-us/articles/204369073
How to escalate a ticket:
https://support.pivotal.io/hc/en-us/articles/203809556
[image: support] <https://support.pivotal.io/> [image: twitter]
<https://twitter.com/pivotal> [image: linkedin]
<https://www.linkedin.com/company/3048967> [image: facebook]
<https://www.facebook.com/pivotalsoftware> [image: google plus]
<https://plus.google.com/+Pivotal> [image: youtube]
<https://www.youtube.com/playlist?list=PLAdzTan_eSPScpj2J50ErtzR9ANSzv3kl>
Re: Reviews and Conflicts in Pull Requests
Posted by Bruce Schuchardt <bs...@apache.org>.
I don't think the "single commit" notion is even a good one for an
original PR. I've discussed this with other people and we think it's
okay for the PR to have multiple commits if they serve different
purposes such as renaming variables or restructuring code before
attacking a problem. Likewise it's okay to address review comments in
an additional commit. The PR template just addresses the initial PR
being a single commit anyway.
That being said, if the changes required by reviewers are so substantial
that the PR becomes difficult to review it should be pulled and a new PR
should be created. Otherwise who's going to bother with it?
Regarding conflicts, that's between the contributor and the committer.
A PR that picks up conflicts due to other peoples' work isn't invalid.
Most conflicts are easy to resolve but if it's too gnarly would the
contributor want someone else to do the work and maybe mess up?
On 5/22/18 2:41 AM, Ju@N wrote:
> Hello geode devs,
>
> The Geode Wiki has a lot of useful information, not only about the
> usage/internal architecture of Geode, but also explanations and guidelines
> about how Code Contributions [1] should be made. However, some common cases
> are not explained in detail and, as such, contributors (and also reviewers)
> might have a hard time trying to deal with these situations in an
> *standardize* manner. In particular, I'm referring to the following two
> scenarios, which can happen for every single pull request:
>
> - *Reviewers request a change: *do contributors have to make the changes
> and just add another commit to the original pull request?. Do
> contributors have to make changes and squash everything in one single
> commit (executing a *push --force *afterwards)?. Do contributors have to
> close the pull request and open a new one with everything squashed?.
>
>
> - *Pull Request looks good but, after a while without being merged,
> somebody else makes a change in develop, causing the original pull request
> to become conflictive with the current develop branch: *should the
> committer merging the pull request solve these conflicts as well?. Do
> contributors have to solve the conflict locally?, if so, do they have to
> add a new commit to the pull request or squash everything in one single
> commit (executing a *push --force *afterwards)?.
>
> It would be good if we could add these details to the Wiki so everyone
> knows how to proceed whenever they hit this situations (git commands will
> also be helpful). Would anyone be able to add this to the Wiki?, or reply
> to this thread so the approved/recommended process is documented somewhere?.
> Cheers.
>
> [1]: https://cwiki.apache.org/confluence/display/GEODE/Code+contributions
>