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

Simplify pull request template #11563

Merged
merged 6 commits into from Oct 25, 2019
Merged

Simplify pull request template #11563

merged 6 commits into from Oct 25, 2019

Conversation

@adbridge
Copy link
Contributor

adbridge commented Sep 25, 2019

Now looks like:

Description (required)

Summary of change (What the change is for and why)

Condense multiple options into the main three and summarize what
can go into those 3 types.

Add a test results section.

Added additional guidance

Documentation (Details of any document updates required)

Updates required to workflow docs


Pull request type (required)

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results (required)

[x] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers (optional)


Release Notes (required for feature/major PRs)

Summary of changes
Impact of changes
Migration actions required
Condense multiple options into the main three and summarise what
can go into those 3 types.

Add a test results section.

Add compulsory sections to Release Notes.
@adbridge adbridge requested review from bulislaw, 0xc0170 and neilwjackson Sep 25, 2019
@ciarmcom ciarmcom requested a review from ARMmbed/mbed-os-maintainers Sep 25, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Sep 25, 2019

@adbridge, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

[ ] Docs update
[ ] Test update
[ ] Breaking change
[ ] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)

This comment has been minimized.

Copy link
@bulislaw

bulislaw Sep 25, 2019

Member

We should link to contribution guidelines with some further description of these.

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Sep 25, 2019

Member

I believe refactor should go into feature one as stated also in our guidelines

This comment has been minimized.

Copy link
@adbridge

adbridge Sep 25, 2019

Author Contributor

I believe refactor should go into feature one as stated also in our guidelines

Bartek thought it should go to patch unless the actual functionality is changed

This comment has been minimized.

Copy link
@adbridge

adbridge Sep 25, 2019

Author Contributor

We should link to contribution guidelines with some further description of these.

Don't we already do that in the non visible notes?

This section is compulsory for all Feature and Major PR types.
This section is automatically added to release notes. Please fill in each sub-section with sufficient detail for a user.
For more information, please see [the contributing guidelines](https://os.mbed.com/docs/mbed-os/latest/contributing/workflow.html#pull-request-types). 

Although that comes under the Release Notes. Should we maybe put that right at the top or are you saying we should make that note visible?

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Sep 25, 2019

Member

Could be on the top as "Read our guidelines (url reference here) to understand pull request template" or similar note

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Sep 25, 2019

Member

Bartek thought it should go to patch unless the actual functionality is changed

@kjbracey-arm is that OK? I've seen this previously considered as refactors are internal changes (the worst thing there is introducing new bugs, but all backward compatible).

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Sep 25, 2019

Contributor

The reason to not include refactors (or any change not fixing a specific bug) would be to reduce the chance of unneccessarily introducing new bugs.

It increases confidence for users that the patch series is safe to accept general updates from - if they're on 5.14.x, they could feel confident that 5.14.y won't break anything.

In semantic versioning terms, certainly, a refactor would be a patch version increment, as it's supposed to be backwards compatible. But a Linux kernel patch release wouldn't accept it a backported kernel "refactor" unless it was to fix a specific bug, on grounds of risk.

So Bartek or someone needs to make the call on desired stability of backport patch series.

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Sep 25, 2019

Member

@bulislaw Please confirm this change is fine .

Copy link
Member

0xc0170 left a comment

Otherwise looks good !

[ ] Docs update
[ ] Test update
[ ] Breaking change
[ ] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Sep 25, 2019

Member

I believe refactor should go into feature one as stated also in our guidelines

[ ] Breaking change
[ ] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[ ] Feature update (New feature / Functionality change / New API)
[ ] Major update (Breaking change E.g. Return code change / API behaviour change)

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Sep 25, 2019

Member

should we add here - Release notes required , and also for feature update?

This comment has been minimized.

Copy link
@adbridge

adbridge Sep 25, 2019

Author Contributor

There is a comment under the Release Notes section that says it is compulsory for Feature and Major updates

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Sep 25, 2019

Member

There is a comment under the Release Notes section that says it is compulsory for Feature and Major updates

yes, just thinking if I fill this in if I get to Release notes :-) I would leave it in both places

[ ] Test update
[ ] Breaking change
[ ] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[ ] Feature update (New feature / Functionality change / New API)

This comment has been minimized.

Copy link
@jamesbeyond

jamesbeyond Sep 25, 2019

Contributor

If a PR is a feature update, can we add a sub-question to ask the submitter to confirm where is the documentation and tests related to this new feature. @adbridge

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Sep 25, 2019

Member

as if "new API or feature, documentation and tests provided are required" ?

This comment has been minimized.

Copy link
@adbridge

adbridge Sep 25, 2019

Author Contributor

@jamesbeyond We already have a rest results (optional) section there. Are you saying we should expand on that to also show evidence for new feature testing ? I'm happy to do that. We could also have a section for 'Documentation Link'. What do @0xc0170 and @bulislaw think?

This comment has been minimized.

Copy link
@jamesbeyond

jamesbeyond Sep 25, 2019

Contributor

Test Result would make people think the CI that running would provide the test result.
The really question here is: if your PR is about a new feature, where is your tests ? are these test can be executed by current CI ?
Maybe we can make the section compulsory, but let them choose one from bellow?
[ ] No Tests required for this change
[ ] This PR changes will be covered by existing tests and CI
[ ] Tests supplied with in this PR
[ ] Other, specify bellow

BTW, I'd like to have the Documentation link section as well

This comment has been minimized.

Copy link
@adbridge

adbridge Sep 25, 2019

Author Contributor

Mmm the only problem I have with this is we are trying to simplify the basic PR template and this would start to add complexity again. Though I do like the idea @bulislaw @0xc0170 what are your thoughts ?

This comment has been minimized.

Copy link
@jamesbeyond

jamesbeyond Sep 25, 2019

Contributor

I think our goal is trying formalize and clarify the process, make the guidance easy for users to follow, and make it easy for us to manage the PRs. Not just only simplify it 😉

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Sep 25, 2019

Member

The problem with above is for many people CI tests are unkown and even testing infrastructure, they test it via their app or simple test case, so this would be mostly for internal teams rather than all of us.

What we often do is ask a simple question "How was this tested", maybe this type of question to be included in the template - it should make a person think about testing this. The answer could be one of the above "no tests done" , "tested on simple app", "run entire test suite", etc. I still feel we don't test enough would this question asked for all PRs help us to improve test coverage?

This comment has been minimized.

Copy link
@jamesbeyond

jamesbeyond Sep 25, 2019

Contributor

Yes @0xc0170 I agree with you. So the things I am a bit worried is about when people adding new feature, they more focus about the feature and forgot about tests and documentation.
e.g. the baremetal profile. Having this test compulsory section would let aware that tests are required as part of feature.

Also tested on simple app it is not acceptable to me for a new feature. we need them either in formal of Greentea, Unittest format or a test in their own team's CI, unless they'll need a really good explanation why there is no test.
For the changes to a existing feature, it is fairly enough for them to claim this is already covered by a exist greentea or unittest tests

Where is the test? is one of the question, and we know there are limitation about CI, e.g. USB test is is not running. I think that's another issue for CI/Infra team to resolve

Also add section separators
@adbridge

This comment has been minimized.

Copy link
Contributor Author

adbridge commented Sep 25, 2019

Updated to add test results section and separating lines. Note to see them properly you have to look at the file in none edit mode. I've edited the template on this actual PR to demonstrate.

adbridge added 2 commits Sep 25, 2019
Made required/optional italic and removed a couple of superfluous lines
Copy link
Member

0xc0170 left a comment

A comment to "Tests" section only, the rest looks fine to me.

Are "Tests" required or optional? the header states required, then in the comment it is optional?

I am not certain about "covered by existing tests and CI" - most of people have no idea what runs in CI, so would remove "and CI" but that might be just me being picky.

"Tests supplied below" - rather add "Test results" ?

@jamesbeyond

This comment has been minimized.

Copy link
Contributor

jamesbeyond commented Sep 26, 2019

Hi @adbridge @0xc0170

  • No Tests required for this change, shall we add (E.g. docs updates)
  • Covered by existing mbed-os tests (Greentea or Unittest) should be good to me.
  • tests supplied below sounds a bit ambiguous, maybe tests supplied part of this PR ?
  • why we dropped others, explained bellow option? I believe some PR will have some case, where it will not fall into any of the 3 options. like this is tested in their own team's CI, or test is submitted in separate PR.

BTW, how about documentation section? are we still having that section?

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Sep 26, 2019

BTW, how about documentation section? are we still having that section?

Docs covered in the Patch update.

tests supplied below sounds a bit ambiguous, maybe tests supplied part of this PR ?

Tests logs attached ?

why we dropped others, explained bellow option? I believe some PR will have some case, where it will not fall into any of the 3 options. like this is tested in their own team's CI, or test is submitted in separate PR.

If not any of these 3, the text below these 3 options is "others" - does it need own item in the list?

@adbridge

This comment has been minimized.

Copy link
Contributor Author

adbridge commented Sep 26, 2019

If not any of these 3, the text below these 3 options is "others" - does it need own item in the list?

Not sure what 'others' would be? Pretty sure those 3 cover everything. Either no testing, testing covered by us or else they need to supply evidence...

@adbridge

This comment has been minimized.

Copy link
Contributor Author

adbridge commented Sep 26, 2019

@jamesbeyond @0xc0170 @bulislaw please take another look

@0xc0170 0xc0170 self-requested a review Sep 26, 2019
Copy link
Member

0xc0170 left a comment

Not certain about separators

@adbridge

This comment has been minimized.

Copy link
Contributor Author

adbridge commented Sep 26, 2019

Not certain about separators

As in the lines ?

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Sep 26, 2019

I forgot to remove my dated comment , as I firstly checked how it looks in the template then reviewed the file generated, all fine to me.

[] Major update (Breaking change E.g. Return code change / API behaviour change)

----------------------------------------------------------------------------------------------------------------
### Test results (*required*)

This comment has been minimized.

Copy link
@bulislaw

bulislaw Sep 26, 2019

Member

So is it required or optional? It says required in line 28 and optional in 30

This comment has been minimized.

Copy link
@adbridge

adbridge Sep 26, 2019

Author Contributor

Required should be updated, I thought I had changed line 30 in the last update - will check

Optional
In case of breaking changes, functionality changes or refactors, please add release notes here.
For more information, please see [the contributing guidelines](https://os.mbed.com/docs/mbed-os/latest/contributing/workflow.html#pull-request-types).
This section is compulsory for all Feature and Major PR types.

This comment has been minimized.

Copy link
@bulislaw

bulislaw Sep 26, 2019

Member

For the features and major PRs we need only "summary of changes", for breaking changes we need all three subsections.

This comment has been minimized.

Copy link
@adbridge

adbridge Sep 26, 2019

Author Contributor

Major update is a breaking change?

This comment has been minimized.

Copy link
@adbridge

adbridge Sep 26, 2019

Author Contributor

OK updated the comments accordingly

Copy link
Contributor

jamesbeyond left a comment

Looks good to me, thanks!

@0xc0170 0xc0170 removed the needs: review label Sep 30, 2019
@0xc0170 0xc0170 added the needs: CI label Sep 30, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Sep 30, 2019

CI started

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Sep 30, 2019

Looks like this is ready, just need docs update reference here.

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 25, 2019

Docs PR approved (ARMmbed/mbed-os-5-docs#1158), I'll merge this one

@0xc0170 0xc0170 merged commit e5a728f into ARMmbed:master Oct 25, 2019
14 checks passed
14 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 9003 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.