Discussion for enriching the PR template

[mdelapenya]

We'd like to enrich the existing PR template adding a few sections that we consider helpful during the review process for a PR, making it even better.

One important thing for us is not to overwhelm whoever is filling out this template, so please feel free to discuss about removing parts of it if that's the case.

In this template, besides the checklist that we already have, we'd like to define the following structure, where:

• each section is a H2 element
• the mandatory/recommended/optional keyword goes in an HTML comment (so won't appear in the end description)
• the description goes in an HTML comment (so won't appear in the end description)

You could see examples for Beats here: elastic/beats#15388 and elastic/beats#15422

The template:

Type of change (Mandatory)

Please delete options that are not relevant

• New feature
• Bug fix
• Enhancement
• Breaking change

Do you see any other type?

What does this PR do? (Mandatory)

Explain here the changes you made on the PR. Please explain the WHAT: patterns used, algorithms implemented, design architecture, message processing, etc.

Why is it important? (Mandatory)

Explain here the WHY, or the rationale/motivation for the changes.

Checklist (Mandatory)

Add a checklist of things that are required to be reviewed in order to have the PR approved
List here all the items you have verified BEFORE sending this PR. Please DO NOT remove any item, striking through those that do not apply.

Here we have a default list, that could be merged with the existing one:

• I have signed the Contributor License Agreement.
• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• New and existing integration tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules
• Update CHANGELOG.asciidoc
• Update supported-technologies.asciidoc
• Added an API method or config option? Document in which version this will be introduced.
• Added an instrumentation plugin? How did you make sure that old, non-supported versions are not instrumented by accident?

Author's Checklist (Recommended)

Add a checklist of things that are required to be reviewed in order to have the PR approved

How to test this PR locally (Recommended)

Explain here how this PR will be tested by the reviewer: commands, dependencies, steps, etc.

Related issues (Recommended)

Link related issues below. Insert the issue link or reference after the word "Closes" if merging this should automatically close it.

• Closes #ISSUE_ID
• Relates #ISSUE_ID
• Superseeds #ISSUE_ID

Use cases (Recommended)

Explain here the different behaviors that this PR introduces or modifies in this project, user roles, environment configuration, etc.
If you are familiar with Gherkin test scenarios, we recommend its usage: https://cucumber.io/docs/gherkin/reference/

Logs (Recommended)

Paste here output logs discovered while creating this PR, or any other output you consider important to be shared with the team.

Screenshots (Optional)

Add here screenshots about how the project will be changed after the PR is applied. They could be related to web pages, terminal, etc, or any other image you consider important to be shared with the team.

[eyalkoren]

@mdelapenya Thanks for putting efforts into that.
In general, we try to make contribution more friendly, and I think that making the PR template so verbose doesn't support this effort.
Since we often look at PRs within a list, or as a Zube card, a label should indicate its type and its name should cover what it does. If it's not the case it should, so maybe the template should tell the user to pick a label and a proper name, instead of making those mandatory sections. In addition, in many (or most) cases the Why is it important? question would be redundant as long as the name is proper (eg "Add support for Kafka streams" or "Fix ApacheHttpClient instrumentation"), so I wouldn't make it mandatory either.

As for the checklist- the I have signed the Contributor License Agreement checkbox is very useful for external contributors, so even if irrelevant for us, I don't mind removing it when doing a PR. Other than that, I would keep the checklist shorter, so that it won't be too intimidating.

[mdelapenya]

@mdelapenya Thanks for putting efforts into that.
In general, we try to make contribution more friendly, and I think that making the PR template so verbose doesn't support this effort.

Yeah, I totally agree. With this template we don't want to mandatorily add those sections. It's a proposal we consider it valid to gather as much information for the review process.

Since we often look at PRs within a list, or as a Zube card, a label should indicate its type and its name should cover what it does. If it's not the case it should, so maybe the template should tell the user to pick a label and a proper name, instead of making those mandatory sections.

I like the idea! We can explain instead how to label this issue. Thanks!

In addition, in many (or most) cases the Why is it important? question would be redundant as long as the name is proper (eg "Add support for Kafka streams" or "Fix ApacheHttpClient instrumentation"), so I wouldn't make it mandatory either.

I get your point for the title Vs "Why is it important" discussion. While the title uses to be regularly short, this section brings the opportunity to explain the purpose very detailedly, like in the second part of a git commit. That's why we consider the "What" and "Why" sections very important.

As for the checklist- the I have signed the Contributor License Agreement checkbox is very useful for external contributors, so even if irrelevant for us, I don't mind removing it when doing a PR. Other than that, I would keep the checklist shorter, so that it won't be too intimidating.

Totally agree in this point about checklist length. Let's try to keep it simple, and all the same time trying to get as much information about the topics that could cause a quality loss when they are missing: running the tests, don't forget to update the docs, code format, etc. I'd appreciate here if you help us in defining the checklist.

For the other sections, which of them would you keep because they seems valid for you?

[eyalkoren]

I like the idea! We can explain instead how to label this issue. Thanks!

Maybe also a hint as to how to name the PR.

I'd appreciate here if you help us in defining the checklist.

I suggest we start by removing Any dependent changes have been merged and published in downstream modules. I think it is not often a problem and easy to detect when not (fail to merge, fail to compile etc.).
My code follows the style guidelines of this project - should come with a link to such guidelines, which should include something about internal documentation, so I have commented my code, particularly in hard-to-understand areas can be removed as well.

Other (recommended and optional) sections seem useful to me. The idea is that people will be required to do as little as possible stuff that don't directly contribute to improvement.

[mdelapenya]

I like the idea! We can explain instead how to label this issue. Thanks!

Maybe also a hint as to how to name the PR.

I'd appreciate here if you help us in defining the checklist.

I suggest we start by removing Any dependent changes have been merged and published in downstream modules. I think it is not often a problem and easy to detect when not (fail to merge, fail to compile etc.).
My code follows the style guidelines of this project - should come with a link to such guidelines, which should include something about internal documentation, so I have commented my code, particularly in hard-to-understand areas can be removed as well.

Other (recommended and optional) sections seem useful to me. The idea is that people will be required to do as little as possible stuff that don't directly contribute to improvement.

Yeah totally agree. Being an open source project we have to make contributions super smooth. That's my concern too.

I can elaborate a PR based on the above structure, BUT:

• Type of change is moved to labels. We are going to add a comment on how to add the labels

• What and Why, stay

• Checklist stays, but in this format:

• I have signed the Contributor License Agreement.

• I have performed a self-review of my own code

• I have made corresponding changes to the documentation

• My changes generate no new warnings

• I have added tests that prove my fix is effective or that my feature works

• New and existing unit tests pass locally with my changes

• New and existing integration tests pass locally with my changes

• Update CHANGELOG.asciidoc

• Update supported-technologies.asciidoc

• Added an API method or config option? Document in which version this will be introduced.

• Added an instrumentation plugin? How did you make sure that old, non-supported versions are not instrumented by accident?

• The rest of the sections stay as optional/recommended, although we will make an effort to remark in the template (initial description?) they can be removed/skipped.

Thanks for such a great feedback @eyalkoren, it's..

awesome!

[felixbarny]

From my perspective, we can also remove these boxes:

My changes generate no new warnings

We're currently not super-strict with warnings, especially around Javadocs.

New and existing integration tests pass locally with my changes

Integration tests take quite a while to run and require downloading loads of docker containers. I usually never execute those locally, unless I expect issues in a specific one. Normally, I offload that to Jenkins. Sometimes I'm even doing that for unit tests but it still might make sense to leave that step there. However, adding a link to https://github.com/elastic/apm-agent-java/blob/master/CONTRIBUTING.md#testing would be nice.

In general, we don't often have the case that we get a PR and we don't know about what it does or why it is important. Usually, we have a discussion in an issue before a PR is raised and I think this is how it's supposed to be. Just as months of coding can save hours of planning, it makes sense, especially for external contributors, to first check with us whether we are working on that ourselves or in which direction the change should be going. So maybe, instead of having the What does this PR do and Why is it important? sections, require that there's an issue before submitting a PR. However, for small internal changes or even small changes from external contributors, such as fixes for spelling mistakes in the docs, there's no need for a separate issue.

[mdelapenya]

From my perspective, we can also remove these boxes:

My changes generate no new warnings

Cool! We'll remove this one too.

We're currently not super-strict with warnings, especially around Javadocs.

New and existing integration tests pass locally with my changes

Integration tests take quite a while to run and require downloading loads of docker containers. I usually never execute those locally, unless I expect issues in a specific one. Normally, I offload that to Jenkins. Sometimes I'm even doing that for unit tests but it still might make sense to leave that step there. However, adding a link to https://github.com/elastic/apm-agent-java/blob/master/CONTRIBUTING.md#testing would be nice.

Sounds great. In the same PR we can add that to the contributing guidelines! Finding this gap is also part of the effort to get even more quality :)

In general, we don't often have the case that we get a PR and we don't know about what it does or why it is important. Usually, we have a discussion in an issue before a PR is raised and I think this is how it's supposed to be. Just as months of coding can save hours of planning, it makes sense, especially for external contributors, to first check with us whether we are working on that ourselves or in which direction the change should be going. So maybe, instead of having the What does this PR do and Why is it important? sections, require that there's an issue before submitting a PR. However, for small internal changes or even small changes from external contributors, such as fixes for spelling mistakes in the docs, there's no need for a separate issue.

Mmmm... What would you think about having an issue/discussion template with the WHAT/WHY parts, and indicate in the PR that the issue must exist?

[felixbarny]

Yes, we could maybe add it to https://github.com/elastic/apm-agent-java/blob/master/.github/ISSUE_TEMPLATE/Feature_request.md