Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Guide for Committers doc #7175

Merged
merged 1 commit into from Oct 11, 2019
Merged

Conversation

dsouzai
Copy link
Contributor

@dsouzai dsouzai commented Sep 20, 2019

Based on https://github.com/eclipse/omr/blob/master/doc/GuideForCommitters.md

I removed the following: These points are now in the doc based on #7175 (comment) and #7175 (comment)

* If a pull request modifies the [Contribution Guidelines](https://github.com/eclipse/openj9/blob/master/CONTRIBUTING.md),
request the author to post a detailed summary of the changes on the
`openj9-dev@eclipse.org` mailing list after the pull request is merged.

* Do not merge a pull request if the Assignee is someone other than yourself.

* If you will be the primary committer, change the Assignee of the pull request
to yourself.  Being the primary committer does not necessarily mean you have to
review the pull request in detail.  You may delegate a deeper technical review
to a developer with expertise in a particular area by `@mentioning` them.  Even
if you delegate a technical review you should look through the requested changes
anyway as you will ultimately be responsible for merging them.

* Regardless of the simplicity of the pull request, explicitly Approve the
changes in the pull request.

@dsouzai
Copy link
Contributor Author

dsouzai commented Sep 20, 2019

Might need to add something along the lines of ensuring [ci skip] is only added for doc only commits; related would be not running builds for doc only PRs.

doc/GuideForCommitters.md Outdated Show resolved Hide resolved
doc/GuideForCommitters.md Outdated Show resolved Hide resolved
@keithc-ca
Copy link
Contributor

Based on https://github.com/eclipse/omr/blob/master/doc/GuideForCommitters.md

I removed the following:

I misread that the first time as 'added' not 'removed', and agreed those points should be included. There isn't a reference to the OMR guidelines which would make them redundant, so why are they removed?

doc/GuideForCommitters.md Outdated Show resolved Hide resolved
doc/GuideForCommitters.md Outdated Show resolved Hide resolved
@fjeremic
Copy link
Contributor

fjeremic commented Sep 23, 2019

Based on https://github.com/eclipse/omr/blob/master/doc/GuideForCommitters.md

I removed the following:

I misread that the first time as 'added' not 'removed', and agreed those points should be included. There isn't a reference to the OMR guidelines which would make them redundant, so why are they removed?

I too think the removed points should be included. Having had experience on the OMR side I generally find the two middle points to be quite helpful from both a contributor and a committer point of view:

* Do not merge a pull request if the Assignee is someone other than yourself.

* If you will be the primary committer, change the Assignee of the pull request
to yourself.  Being the primary committer does not necessarily mean you have to
review the pull request in detail.  You may delegate a deeper technical review
to a developer with expertise in a particular area by `@mentioning` them.  Even
if you delegate a technical review you should look through the requested changes
anyway as you will ultimately be responsible for merging them.

These effectively give ownership of the PR to a single person who may have context about the PR which other committers may not be aware about. This includes coordination with other repositories (OMR, ibmruntimes, OpenJDK, Adopt, etc.). It also gives the contributor a single person to "nudge" in case all the reviews in the PR have been addressed and the contributor is effectively waiting for a committer to pick up the PR and drive it into the repository.

@dsouzai
Copy link
Contributor Author

dsouzai commented Oct 7, 2019

There isn't a reference to the OMR guidelines which would make them redundant, so why are they removed?

No real reason other than the fact that I hadn't seen us committers do that on OpenJ9. However, seeing as there's interest to be more rigorous, I'll add those in.

@dsouzai
Copy link
Contributor Author

dsouzai commented Oct 7, 2019

@fjeremic @keithc-ca @DanHeidinga Updated the doc with the requested changes; could you take a look again?

doc/GuideForCommitters.md Outdated Show resolved Hide resolved
doc/GuideForCommitters.md Outdated Show resolved Hide resolved
doc/GuideForCommitters.md Outdated Show resolved Hide resolved
doc/GuideForCommitters.md Outdated Show resolved Hide resolved
@dsouzai
Copy link
Contributor Author

dsouzai commented Oct 7, 2019

Updated with requested changes.

@dsouzai
Copy link
Contributor Author

dsouzai commented Oct 9, 2019

I figure we can wait until Friday to see if there are any other comments/concerns before merging as I've brought this up on slack multiple times now. Does that sound reasonable @fjeremic ?

@fjeremic
Copy link
Contributor

fjeremic commented Oct 9, 2019

Does that sound reasonable @fjeremic ?

Agreed. Will merge by EOW unless other review requests are made.

Copy link
Member

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

[ci skip]

Based on https://github.com/eclipse/omr/blob/master/doc/GuideForCommitters.md

Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>
@fjeremic
Copy link
Contributor

I think we are good to go here. Thanks @dsouzai!

@fjeremic fjeremic merged commit 7dd51f6 into eclipse-openj9:master Oct 11, 2019
@dsouzai dsouzai deleted the committerDoc branch October 15, 2019 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants