Re: [DISCUSS]: Commit guidelines for PRs

[Zoltan Haindrich]

Hey,

There are quite a few things which have changed lately...and the github merge 
option have really twisted a few things :)

On 6/30/20 9:31 PM, Vihang Karajgaonkar wrote:

1. Whether to standardize on Squash into one commit

for the PR?


I think squash-and-merge perfectly fits our way of commiting patches (1 PR = 1 Jira ticket). Meanwhile it also opens up the possiblity for contributors to use micro-commits 
while they are developing the change - which could be easier to follow commit-by-commit.

This seemed to me like a win-win :D

To prevent accidental merges or rebases - I've disabled them in the asf.yaml a 
few weeks ago
https://github.com/apache/hive/blob/master/.asf.yaml


2. What are the commit message guidelines? Our project has unfortunately
not been great in documenting the commit message appropriately. Current
guidelines are to have one line commit message and the JIRA is expected to
have more detailed information. However, most of the time the JIRAs  don't
have enough information. I think it would be good to add a few lines of
description as part of the git commit message. Some projects recommend 50/72
formatting

for
the git commit message which I feel is nice.


I don't have a strong opinion about providing a detailed description - maybe we 
should start writing them
But in general it would be great if jira descriptions would also be more specific - I think around 50% of the jiras are created without filling in the description. There 
are cases in which it is obvious what will happen; but that's not always the case


It would be a good idea to fill in some details about the change when the PR is being opened - around that time (since there is already a patch) - he or she might be able 
to give a brief description


slightly connected to this is that we have a HowToCommit wiki page: 
https://cwiki.apache.org/confluence/display/Hive/HowToCommit
which describes to add "(X reviewed by Y)" to the commit messages - since we are now using PRs which can be opened back up again with all the reviewer/etc informations I 
think this will not really add relevant information to the commit.

In case the PR is merged on the github page:
* the author's name and email address is filled according to who opened the PR
* a reference is place in the commit to the PR; which will show the reviews the 
PR got back


3. Do committers merge the PR directly from the github? I am not sure if
there is a way for our committer credentials to be integrated in github.
Otherwise, the other option could be that the committer checks out the PR
and merges it manually into the master branch.


I can only add a helpful link after Peter's explanation :D
https://cwiki.apache.org/confluence/display/OPENWHISK/Accessing+Apache+GitHub+as+a+Committer



cheers,
Zoltan

Re: [DISCUSS]: Commit guidelines for PRs

[Peter Vary]

Hi Vihang,

As you, I like the new infra very much!

I have opinion /answer for 2 of your questions below:

VK > 1. Whether to standardize on Squash into one commit

I think we should squash. The final code should not be polluted with the
meandering way we sometimes arrive to the final solution in some patch
processes with multiple reviewers.

VK > 3. Do committers merge the PR directly from the github?

Yes, I do. There is a possibility to connect your apache account to the
github account. You have to set-up 2 factor authentication for that. I
believe one of David Mollitor's previous letter contains more info on that.

I do not have that stong opinion on question 2. I believe that if the title
of the jira is good enough then it should be enough as a commit message
too. OTOH the PR/Jira discussion should contain the reasoning/debate behind
the scenes. But probably that's just me already adjusted to the status quo
:)

Thanks, Peter

Vihang Karajgaonkar  ezt írta (időpont: 2020. jún.
30., Ke 21:31):

> Thanks to all who worked on the new testing infrastructure. It definitely
> looks like a step up from the older test infrastructure.
>
> I wanted to know if there are any new guidelines for a committer for
> merging the PRs. Earlier we used to create one patch file for each JIRA and
> push it to the master branch. With PRs it is possible that a
> contributor publishes multiple commits (eg. to address review comments). I
> would like to start a discussion on what should be the guidelines on
> merging the PR requests?
>
> Most of you are probably already following it but it would be good to
> formalize the following:
>
> 1. Whether to standardize on Squash into one commit
> <
> https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/about-pull-request-merges#squash-and-merge-your-pull-request-commits
> >
> for the PR?
> 2. What are the commit message guidelines? Our project has unfortunately
> not been great in documenting the commit message appropriately. Current
> guidelines are to have one line commit message and the JIRA is expected to
> have more detailed information. However, most of the time the JIRAs  don't
> have enough information. I think it would be good to add a few lines of
> description as part of the git commit message. Some projects recommend
> 50/72
> formatting
> <
> https://stackoverflow.com/questions/2290016/git-commit-messages-50-72-formatting
> >
> for
> the git commit message which I feel is nice.
> 3. Do committers merge the PR directly from the github? I am not sure if
> there is a way for our committer credentials to be integrated in github.
> Otherwise, the other option could be that the committer checks out the PR
> and merges it manually into the master branch.
>
> Thanks,
> Vihang
>