Skip to content

Add a pull request template providing valuable information when reviewing a PR #15388

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.

Sign up for GitHub

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

Merged
mdelapenya merged 8 commits into from
Jan 16, 2020
Merged

Add a pull request template providing valuable information when reviewing a PR #15388

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
59ae739
Add a PR template that provides valuable information when reviewing a PR
mdelapenya Jan 8, 2020
70a5d05
Add CLA check
mdelapenya Jan 8, 2020
8ae6cdb
Fix typo
mdelapenya Jan 8, 2020
de75df0
Address comments during review
mdelapenya Jan 14, 2020
dc7c628
SF: Fix typo
mdelapenya Jan 15, 2020
de02577
Add deprecation as PR type
mdelapenya Jan 15, 2020
bdd5d0a
Make it clear how to strike through in markdown
mdelapenya Jan 15, 2020
c05e654
Add default configuration files to the checklist
mdelapenya Jan 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions .github/PULL_REQUEST_TEMPLATE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
## Type of change

<!-- Mandatory
Please delete options that are not relevant
-->

- [ ] New feature
- [ ] Bug fix
- [ ] Enhancement
- [ ] Breaking change

@urso urso Jan 9, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Because Beats is also a framework we do also distinguish between User changes and Developer changes (if some "public" API is changed).

@mdelapenya mdelapenya Jan 9, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

When you say a public API changes, you mean that it could break semver with a breaking change? If that's the case, should we keep Breaking change as an item, or should we distinguish between these two: User changes and Developer changes? I personally like the first one, because in my mind it directly connects with an API change that client code must be aware of.

I just did give the current template a try: #15422

For the testing part I wonder if we want to encourage people to use headers for different scenarios/grouping. Do we plan to use the How to test this PR locally? section for the test plan?

I'd say that both the review process and the test plan would benefit of having this kind of information. Besides that, explaining how to test something new would make us realise how easy/difficult is to exercise the new code. And not only that, tech writers and end users would read in that section how the software behaves, which is great in terms of gathering expectations.

About multiple test scenarios/groupings, yes, having subheaders (H3, H4...) will definitively help. We can add that to the comments to provide more info

@urso urso Jan 13, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

When you say a public API changes, you mean that it could break semver with a breaking change? If that's the case, should we keep Breaking change as an item, or should we distinguish between these two: User changes and Developer changes? I personally like the first one, because in my mind it directly connects with an API change that client code must be aware of.

I wonder what is the motivation using these fields? e.g. could labels also do? Do you plan to process these fields somehow in the future?

We don't really follow semver, because we have to follow common Elastic Stack product versioning. Which means that even minor versions can have breaking changes in the API. We track two changelogs in the Beats project. The actual user facing changelog (Changelog.asciidoc and Changelog.next.asciidoc), and a developer facing one (Changelog-developer.asciidoc and Changelog-developer.next.asciidoc). The developer one tracks changed/fixes/added/breaking.

PRs in Beats sometimes span multiple products. Changes can be developer + user facing at the same time or not at all. That is PRs are not always this well separable by product.

e.g. We can have a 'New feature' in some Beats only while having a 'breaking change' and/or 'new feature' in some API community authors might use in their own Beats.

@mdelapenya mdelapenya Jan 13, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wonder what is the motivation using these fields? e.g. could labels also do? Do you plan to process these fields somehow in the future?

Having labels could work too. My preference about having the fields is that is less forgettable than the labels. Anyway, Labels could be used to track issue/pr types.

We don't really follow semver, because we have to follow common Elastic Stack product versioning. Which means that even minor versions can have breaking changes in the API. We track two changelogs in the Beats project. The actual user facing changelog (Changelog.asciidoc and Changelog.next.asciidoc), and a developer facing one (Changelog-developer.asciidoc and Changelog-developer.next.asciidoc). The developer one tracks changed/fixes/added/breaking.

PRs in Beats sometimes span multiple products. Changes can be developer + user facing at the same time or not at all. That is PRs are not always this well separable by product.

e.g. We can have a 'New feature' in some Beats only while having a 'breaking change' and/or 'new feature' in some API community authors might use in their own Beats.

I guess this behavior would apply to labels too 🤔

In any case, we are free to modify/remove any section in this proposal. The goal is to create template which provides great context, fitting Beats unique SDLC. So feel free to suggest adapting the Type section in the manner it works better for the team.

@urso urso Jan 14, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If we don't plan to use it for automating the release notes I think we should keep it more simple (meaning the way you proposed). The only type I'm missing here is 'Deprecation'.

Having labels could work too. My preference about having the fields is that is less forgettable than the labels.

+1

@exekias exekias Jan 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about requiring these labels to be present with a bot? I've seen this in other projects and works fairly well. Good message why the PR is not passing + instructions on how to add the right label

@mdelapenya mdelapenya Jan 14, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We could add a comment about how to properly label the PR.

About automating the release notes/changelog, this is a topic we the @elastic/observablt-robots team are working on. We are trying to get a good automated process for that, but not there yet.

@mdelapenya mdelapenya Jan 14, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The bot can add a label based on multiple things. I can imagine:

  • if a modified path, then add label Foo (we created this bot!)
  • if modified path, then ping Foo user/team
  • if not following a structure, then comment the PR
  • ...


## 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.
-->

- [ ] I have signed the [Contributor License Agreement](https://www.elastic.co/contributor-agreement/).
- [ ] My code follows the style guidelines of this project
- [ ] I have performed a self-review of my own code
Comment thread
mdelapenya marked this conversation as resolved.
Outdated
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
Comment thread
urso marked this conversation as resolved.
- [ ] 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

@exekias exekias Jan 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CI should answer these questions

@mdelapenya mdelapenya Jan 14, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well, we's expect the sender to run the tests locally first, right? At least the unit tests

@urso urso Jan 14, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Once the PR is created it will be run by CI anyways. I think it is good practice to run unit tests before pushing the PR, but in the end CI will tell us the truth.

I like the idea of the check list, so to not forget some common tasks (e.g. have you updated docs/configs). But I would prefer have the check list small. Things that are automatically checked anyways don't need to be in the list.

@mdelapenya mdelapenya Jan 14, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed in the shortness of the checklist is a must. We'll remove what is not necessary.

About the unit test part, I'd suggest keeping it, or at least point to the place where it explains how to run them with a nice experience.

- [ ] Any dependent changes have been merged and published in downstream modules
Comment thread
mdelapenya marked this conversation as resolved.
Outdated
- [ ] New dependencies have been vendorised

@mdelapenya mdelapenya Jan 8, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'd like the team to discuss about the items in the checklist

@urso urso Jan 14, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We should add I have made corresponding changes in the default and reference configuration files.

How about thinning out this list as well. Each PR should be run via CI. Only if CI is green shall the PR be merged. The intake job calls make check to validate style (go fmt, autopep8, go vet). Running integration tests requires some time. Although it is appreciated I would not expect devs to run the complete test suite (up to integration tests) upfront.

Items I personally would remove from the list:

  • New and existing unit tests pass locally with my changes : the moment we open the PR CI will run tests anyways. Everyone will be able to see if unit tests pass.
  • New and existing integration tests pass locally with my changes : Same as unit tests
  • New dependencies have been vendorised : The build/CI will fail anyways if that is not the case.

Items I might consider to remove from the list:

  • I have signed the : If the CLA checker fails it should print a message + link anyways.
  • My changes generate no new warnings. : The go compiler doesn't really generate warnings (it's more oppioninated). And even for C/C++ I'm in favor of using -werror :) . Any other warnings we have in mind?

What exactly is meant by Any dependent changes have been merged and published in downstream modules? Would CI be able to pass if this is not the case?


## 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 #123
- Relates #123
- Superseeds #123
Comment thread
urso marked this conversation as resolved.
Outdated
-->
-

## 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/
-->

## 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.
-->

## Logs

<!-- Recommended
Paste here output logs discovered while creating this PR, or any other output you consider important to be shared with the team.
Comment thread
mdelapenya marked this conversation as resolved.
Outdated
-->