Discussion for enriching the Github templates

[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 fact we received very positive feedback from the Java team to adapt to their needs. The same applies here 😄

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 texts go in an HTML comment (so won't appear in the end description)

You could see examples for Beats, and APM Agent Java

We will also use this markdown format (H2 and HTML comments) for the other templates you already have.

The initial template: ⬇️⬇️⬇️⬇️

A few suggestions about filling out this PR

1. Use a descriptive title for the PR.
2. If this pull request is work in progress, create a draft PR instead of prefixing the title with WIP.
3. Please label this PR at least one of the following labels, depending on the scope of your change:

• feature request, which adds new behavior
• bug fix
• enhancement, which modifies existing behavior
• breaking change

4. Remove those recommended/optional sections if you don't need them. Only "What does this PR do" and "Checklist" are mandatory.
5. Submit the pull request: Push your local changes to your forked copy of the repository and submit a pull request (https://help.github.com/articles/using-pull-requests).
6. Please be patient. We might not be able to review your code as fast as we would like to, but we'll do our best to dedicate to it the attention it deserves. Your effort is much appreciated!

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, based on Beats and Java/Ruby agents:

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. (Just in case, strikethrough uses two tildes. Scratch this.)

• I have signed the Contributor License Agreement.
• My code follows the style guidelines of this project
• I have rebased my changes on top of the latest master branch
  Update your local repository with the most recent code from the main repo, and rebase your branch on top of the latest master branch. We prefer your initial changes to be squashed into a single commit. Later, if we ask you to make changes, add them as separate commits. This makes them easier to review.
• I have made corresponding changes to the documentation
• 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
  Run the test suite to make sure that nothing is broken. See https://github.com/elastic/apm-server/blob/master/TESTING.md for details.
• I have updated CHANGELOG.asciidoc

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
• Requires #ISSUE_ID
• Superseds #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. It could be a stacktrace, logs of a library...

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.

[simitt]

I am not quite certain what the difference between Checklist and the Author's Checklist is - could you explain that a bit more?

[mdelapenya]

The main difference comes from the perspective: meanwhile Checklist is fixed and defined by the project, Author's checklist is something to be provided by the sender, not present in the original one.

I'm seeing the second one as optional in other teams, so feel free to discard this section (or any other) in the template.

[axw]

Please label this PR at least one of the following labels, depending on the scope of your change:
feature request, which adds new behavior

You mean "feature"? Feature request would make sense for issues.

I'm not sure I see how these labels (particularly feature/enhancement) are useful for PRs anyway. Do you have use cases in mind? They're great for issues - for backlog prioritisation, etc.

[axw]

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.

I think this is not pertinent to most PRs. If you're doing a big refactoring, sure - explain the new architecture and the reasoning behind it. If you're introducing a new messaging pattern, explain the implications on concurrency, etc.

(On the topic of refactoring, I'd like to add a suggestion that any significant refactoring or style changes are made independently of functional changes. This allows reviewers focus appropriately.)

I think we should combine the "What" and "Why" sections into just "Summary" or similar. It's more helpful to lead with the motivation, since it assists the reviewer in understanding the importance of the changes while reading through them.

If the changes are self-evident, then it's not helpful to repeat it in the description when we can look at the code. It can even be problematic - if the code ends up changing during review, and the description doesn't get updated, then when coming back to the PR to understand the change in the future it may be very confusing.

Suggestion:

Motivation and summary of changes

Please explain the motivation behind this PR, along with a summary of the major changes involved.

[axw]

"Author's checklist" might be useful for people to convey some notes to the reviewer. I think we should start minimal (i.e. without it), and iterate.

[axw]

Finally:

• I don't think screenshots are applicable in many PRs, so I'd drop that from the template.
• Maybe change "Logs" to "Sample output", but really I think we should start without it and iterate.
• "Use cases" -- doesn't this belong more to a specification issue? I think we should remove this too.

Which would leave:

1. Motivation/summary
2. Checklist
3. How to test
4. Related issues

[mdelapenya]

Let me send a PR with this feedback :)

Thank you all!