CASSANDRA-17837 Add pull request template and update readme #1799
Conversation
I think this should instead read something like: Commit message is in the format of: |
Say more? Preference thing, or functional thing, or something else? |
Multiple requests on the dev thread was to remove it. We don't want "actions" required in the template, only useful info. |
@josh-mckenzie , the project's precedence today (as far as I have observed) is any additional descriptive paragraph goes in the middle, like |
... I... well yes. Of course it does. I need to stay off github until I've had my morning ☕ |
|
Text changed to read: Asscociated IssueReplace this text with the Jira issue resolved (e.g. CASSANDRA-####) text specifying why this is a trivial patch. Author(s) and ReviewersPatch by ; reviewed by <reviewer[s]> Co-authored-by: author1 author1@domain.com, author2 author2@domain.com, ..., authorN authorN@domain.com What the patch doesReplace this text with a detailed commit paragraph of why and how the change was made and relevant context in a paragraph here Recommendations:
NoticeBy submitting this pull request, I acknowledge that I am making a contribution to the Apache Software Foundation under the terms and conditions of the Contributor's Agreement. ###References: |
|
|
|
Co-authored syntax is: |
|
|
|
|
|
|
.github/pull_request_template.md
Outdated
|
|
||
| - Commits should be squashed to remove intermediate development commit messages. | ||
| - Key commit messages start with the issue number (CASSANDRA-xxxx) | ||
| - if not a trivial change: |
There was a problem hiding this comment.
personally I would prefer we don't have this line and leave figuring out if trivial to the committers, so keep line 20 to 22, and remove the indent. What is trivial for one may not be for another, so defaulting to best practice with committers being able to allow special cases for trivial work.
There was a problem hiding this comment.
+1 to removing this. What is and what is not trivial is up the committer ultimately.
There was a problem hiding this comment.
While committer is the ultimate authority, the author is making a request. If the author believes it to be trivial the author should state why. If committer agrees then it is trivial. This is the opportunity for the author to make the case that the change is trivial. If the change is trivial then there may not be a JRIA ticket, there should not be documentation changes (unless it is a documentation typo) and there should not be additional testing required. If any of those exist then the change is probably not trivial.
See now that the squash comment and the commit messages start with are not appropriate and will remove them.
|
Updated:
Added the missing word "or". Reformatted Co-authored lines Asscociated IssueReplace this text with the Jira issue resolved (e.g. CASSANDRA-####) or text specifying why this is a trivial patch. Author(s) and ReviewersPatch by <author>; reviewed by <reviewer[s]> Co-authored-by: First Last author1@domain.com What the patch doesReplace this text with a detailed commit paragraph of why and how the change was made and relevant context. Recommendations:if not a trivial change:
NoticeBy submitting this pull request, I acknowledge that I am making a contribution to the Apache Software Foundation under the terms and conditions of the Contributor's Agreement. ###References: |
.github/pull_request_template.md
Outdated
|
|
||
| ## Author(s) and Reviewers | ||
|
|
||
| Patch by <author>; reviewed by <reviewer[s]> |
There was a problem hiding this comment.
needs to be correct to match that found in https://cassandra.apache.org/_/development/how_to_commit.html
There was a problem hiding this comment.
So this should literally read:
<One sentence description, usually Jira title or CHANGES.txt summary>
<Optional lengthier description>
patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####
Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>
@michaelsembwever What if there is no Jira ticket? What do we put in for a trivial pull?
.github/pull_request_template.md
Outdated
|
|
||
| ## What the patch does | ||
|
|
||
| Replace this text with a detailed commit paragraph of why and how the change was made and relevant context. |
There was a problem hiding this comment.
i say remove. it's in the commit message already.
There was a problem hiding this comment.
It is not in the commit if the pull request is not a patch. (like this one)
There was a problem hiding this comment.
i don't understand. how do you create a pull request without a commit?
.github/pull_request_template.md
Outdated
|
|
||
|
|
||
| ## Notice | ||
| By submitting this pull request, I acknowledge that I am making a contribution to the Apache Software Foundation under the terms and conditions of the Contributor's Agreement. |
There was a problem hiding this comment.
i say remove this. it is not required.
.github/pull_request_template.md
Outdated
|
|
||
| ###References: | ||
| * The [Apache Cassandra "Contributing to Cassandra" guide](https://cassandra.apache.org/_/development/index.html) | ||
| * The [Apache Cassandra "Working on Documentation" guide](https://cassandra.apache.org/_/development/documentation.html) |
There was a problem hiding this comment.
remove this, it's already part of the previous link.
There was a problem hiding this comment.
I think we need to look at this as a possible first point of contact. A new person grabs a ticket, clones the repository, makes a change, submits a pull request. This is the first place the project has an opportunity to introduce the new person to the project standard practices. Providing reference links is a way to do this. Providing deeper links (like the documentation link) is a way to promote contributions in those spaces.
For these reasons I think that these 2 links should stay.
There was a problem hiding this comment.
Disagree, it's duplicating information. The documentation page is embedded in the first link.
There was a problem hiding this comment.
duplicating information <- duplication isn't always bad. The goal here is to provide context and framing for folks to get situated, and so long as we don't literally have redundancy on the front-facing store of information here, I think we should keep it.
There was a problem hiding this comment.
but why the documentation, instead of duplicating any of the other parts of that page? (i would have thought code style was far more valuable to the newcomer).
the very first thing on the index.html page is the list of sections, documentation being one of them.
.github/pull_request_template.md
Outdated
| ###References: | ||
| * The [Apache Cassandra "Contributing to Cassandra" guide](https://cassandra.apache.org/_/development/index.html) | ||
| * The [Apache Cassandra "Working on Documentation" guide](https://cassandra.apache.org/_/development/documentation.html) | ||
| * The [Apache Contributor's Agreement](https://www.apache.org/licenses/contributor-agreements.html). |
There was a problem hiding this comment.
i say remove this. it is not required and will only scare people off.
There was a problem hiding this comment.
I am not sure about scaring people off but, after checking on the legal stance, it seems that the person merging the pull request is required to have the ICLA and be responsible for the merge. So I am happy to remove it.
There was a problem hiding this comment.
I disagree about this scaring people off. The second paragraph in the referenced page is:
These agreements help us achieve our goal of providing reliable and long-lived software products through collaborative, open-source software development. In all cases, contributors retain full rights to use their original contributions for any other purpose outside of Apache, while providing the ASF and its projects the right to distribute and build upon their work.
Which seems completely inclusive and reasonable to me.
There was a problem hiding this comment.
it's out of scope for the PR. we won't actually be asking the contributor for an ICLA. creates unnecessary confusion.
speaking from experience, i don't contribute simple one-liners PRs to other projects if i'm confronted with such a barrage of information. but that's just me.
Here's a crack at a wholesale revision of the line above Claude rather than a point by point poking of holes. 😁Commit MessageReplace this text with a one-line summary of what the patch does Commit DetailsReplace this text with a paragraph detailing why the patch is necessary and any context around it to help understand it and get situated reviewing or maintaining it Author(s) and ReviewersPatch by ; reviewed by <reviewer[s]> [for CASSANDRA-XXXX] Co-author(s)Co-authored-by: First Last author@domain.com Checklist:
References: |
|
For help on contributing pease read our "Contributing to Cassandra" guide Unless trivial we require a JIRA ticket that contains a description of what is fixed, why it is needed, and what branches this is to apply to. Commit messages are expected in the format |
|
The more verbose version holds people by hand and walks them through a park, Mick's version assumes they are all adults who take an umbrella when they see cloudy sky. No offence meant to anybody who crafted the more verbose version, I just find it important to express how this might be looked at. That is my opinion when I see both versions side by side. |
Inclusivity often feels that way. It's definitely "not for me", but I also wouldn't find it off-putting if I came upon a project that erred on the side of smoothing out on-ramps for the developer UX. |
|
Yeah I can see that. Up to us to decide what is better. But when I see "Replace this text with a one-line summary of what the patch does" ... I mean, do we have to be this specific? Anyway my 0.02 :) |
|
Warm and inclusive are important to me. I'm thinking the contributing guide is the better place to be verbose. |
|
What about looking what Kafka, Spark and other few top projects have it like? And just copy the same format / approach / verbosity. |
|
Spark has no checklist and seems to be the most reasonable to me. Kafka, Pulsar and Beam have some kind of mix of checklists and questions. Checklists seems to be quite popular but I would not go with them in Cassandra. For example, for Kafka, they have checklist there for committers. I do not think we need any checklists for ourselves. this is quite verbose to me but the formulation is quite nice, it is not about "change this line here" but "clarify and explain please", I think that is better. https://github.com/apache/spark/blob/master/.github/PULL_REQUEST_TEMPLATE |
Fair point; I use them to work around some personal workflow failings so I view them quite differently. 😁 What do you guys think about adding in a reference to the confluence wiki? We're relying on it for more things and it's got some discoverability problems. I'm viewing PR's as also serving the purpose of "if this is someone's first contact with the project other than the cloned code-base, how can we help them get oriented?" Commit Message: References:
|
Thinking about this a bit more; summarizing the requirements to contribute here helps orient folks rather than deferring those asks to a very long "How to contribute" page (see: https://cassandra.apache.org/_/development/index.html). We're imposing actions on users to integrate with our project culture and agreed upon requirements, whether we do it here or on a longer page we link to from here. Also, I'm all for us having long-form documentation that takes 60+ hits of page down to get through, but we also want to encourage folks to "scratch their own itch" and get correctly oriented here without the steep ask of them reading through the entire "how to contribute" page. If we assumed that people might want to have a minimal interaction w/the project as their first set of interactions that looks something like:
I think it's valuable to have a minimal checklist to let them know that "hey, there's more to this than just opening a PR; maybe check the links in the references section to about running CI". |
yes, as that does not impose actions. The same as my wording "Commit messages are expected in the format". It doesn't tell someone to do something, but it is informative to how we work. I know it's a nuance, but I find it creates a more pleasant tone. |
|
I made changes as per @josh-mckenzie and @smiklosovic it now reads Thanks for sending a pull request! Here are some tips for you: If this is your first time please
Commit messages should follow the following format: |
|
I added a link to the jira: Thanks for sending a pull request! Here are some tips for you: If this is your first time please
Commit messages should follow the following format: The Cassandra Jira The Apache Contributor's Agreement And a similar link in the readme. |
|
I'm still in favour of removing altogether that bottom line: And to nit-pick, I think
could be shorten to |
|
Simplified the first line to as @michaelsembwever suggested. |
Claudenw
changed the title
|
I'm still in favour of removing altogether that bottom line: The Apache Contributor's Agreement. |
|
How do we resolve the issue of the last line in the template? There is at least one person that wants it and at least one that does not. |
|
I think it should be removed as well. |
You need patience and to talk it through and get acceptance from everyone that initially raised any objections/concerns. Or move ahead with it removed (as no one voiced concern about this patch being unacceptable/invalid without it). |
Say more here? (Not that I'm re-opening this can of worms, I'm fine either way. Want to learn here). Is the concern that it'll signal some larger commitment that'll be off-putting for new contributors and we'd rather defer it to later in their contribution process? Or is the ICLA only needed when one becomes a committer on a project and the assignment of copyright and legal usage is implicit with a PR someone opens? |
That is how it went for me. I think that it's best to avoid mentioning legally binding documents unless required. |
It's only required by large contributions (and when becoming a committer). It does not apply to the reader we are primarily targeting here. The onus is on the committer reviewing to catch any such need.
Correct. Our license states it. Significant contributions that add new files, we want to ask the contributor to ensure they have added the license header to the files themselves (which creates the implicit acknowledgement). This is one of the reasons we have the rat check in the build/test process. We probably need to be better at ensuring the committers when reviewing know this. But this PR template text is not the place for that, IMHO. (And nothing on the ICLA page goes into such detail anyway.) |
|
This pull request appears to have reached consensus and been approved. Who can merge it? |
|
@Claudenw , agree. Can you squash and write the commit message please. I will merge once that is done. |
Claudenw
force-pushed
the
CASSANDRA-17837_pull_request_template
branch
from
c3c0065
to
5b239dc
Compare
@michaelsembwever Done. |
umm, in the format as the template documents :) |
Claudenw
force-pushed
the
CASSANDRA-17837_pull_request_template
branch
from
5b239dc
to
6b65034
Compare
README.asc
Outdated
| @@ -9,6 +9,8 @@ https://cwiki.apache.org/confluence/display/CASSANDRA2/DataModel[Row store] mean | |||
|
|
|||
| For more information, see http://cassandra.apache.org/[the Apache Cassandra web site]. | |||
|
|
|||
| Issues should be reported on The https://issues.apache.org/jira/projects/CASSANDRA/issues/[The Cassandra Jira]. | |||
There was a problem hiding this comment.
@Claudenw there is double "The", one after "on" and one in the link.
.github/pull_request_template.md
Outdated
| @@ -0,0 +1,25 @@ | |||
| Thanks for sending a pull request! Here are some tips if you're new here: | |||
|
|
|||
| * Ensure you have added or run the [appropriate tests](https://cassandra.apache.org/_/development/testing.html) for your PR | |||
There was a problem hiding this comment.
Add dot at the end please.
|
I left two last comments otherwise LGTM. |
Claudenw
force-pushed
the
CASSANDRA-17837_pull_request_template
branch
from
6b65034
to
dfb45fd
Compare
patch by claudenw; reviewed by dritfx, dcapwell, josh-mckenzie, michaelsembwever for CASSANDRA-17837
Claudenw
force-pushed
the
CASSANDRA-17837_pull_request_template
branch
from
dfb45fd
to
57d491c
Compare
Pull request Description: Adds pull request template that looks like this form. This should be applied to all released and official development branches.
Issue resolved: CASSANDRA-17837
or
if not a trivial change:
References: