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

pull request: add required info #6231

Merged
merged 1 commit into from Mar 1, 2018

Conversation

Projects
None yet
7 participants
@0xc0170
Member

0xc0170 commented Feb 27, 2018

Description

Please use these 2 sections for describing a pull request. They should be part
of every pull request.

The type specifies what is expected from the pull request and when it can be
released. For instance a feature pull request can't be expected to go to the
patch release.

Important: do not mix pull request types !

I added also required to description (is Required placed there good, or any other suggestions?)

Pull request type

  • Fix
  • Refactor
  • New target
  • Feature
  • Breaking change

@0xc0170 0xc0170 requested review from cmonr, sg-, geky, adbridge and AnotherButler Feb 27, 2018

> Good example: https://os.mbed.com/docs/latest/reference/guidelines.html#workflow (Pull request template)
# Pull request type
> Required; Please tick one of the following types

This comment has been minimized.

@cmonr

cmonr Feb 27, 2018

Contributor

Semicolons?

@@ -1,10 +1,12 @@
# Description
> Detailed changes summary | testing | dependencies
> Required; detailed changes summary | testing | dependencies

This comment has been minimized.

@cmonr

cmonr Feb 27, 2018

Contributor

Is this suppose to read "detailed changes summary" or "detailed changes" "summary | testing | dependencies" ?

This comment has been minimized.

@0xc0170

0xc0170 Feb 27, 2018

Member

Any of these, just examples what to be expected. just that section is required

This comment has been minimized.

@geky

geky Feb 27, 2018

Member

Oh, yes this ends up rendered weird as is. Thoughts on actual comments?

^ comment here ^

@adbridge

Where does a breaking API change that is not a new feature fit here? Should we change 'feature' to
'Breaking change / New feature' ?

> Good example: https://os.mbed.com/docs/latest/reference/guidelines.html#workflow (Pull request template)
# Pull request type
> Required; Please tick one of the following types

This comment has been minimized.

@geky

geky Feb 27, 2018

Member

Why not use actual comments?

https://stackoverflow.com/a/39737760

This comment has been minimized.

@0xc0170

0xc0170 Feb 27, 2018

Member

Good, let me test

This comment has been minimized.

@adbridge

adbridge Feb 27, 2018

Contributor

@geky because we want to be able to automate the checking of the PR type and automatically assign the release tag

@geky

This comment has been minimized.

Member

geky commented Feb 27, 2018

@adbridge, IMO breaking change/new feature should be two separate things.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Feb 27, 2018

@geky I would by happy with a separate entry for 'Breaking change'

@0xc0170 0xc0170 force-pushed the 0xc0170:fix_pr_template_required branch 2 times, most recently from ecdc927 to dcfa6c3 Feb 27, 2018

@0xc0170 0xc0170 force-pushed the 0xc0170:fix_pr_template_required branch from dcfa6c3 to c529b29 Feb 27, 2018

pull request: add required info
Please use these 2 sections for describing a pull request. They should be part
of every pull request.

The type specifies what is expected from the pull request and when it can be
released. For instance a feature pull request can't be expected to go to the
patch release.

Important: do not mix pull request types !

Changed also the heading type, to make it smaller.

@0xc0170 0xc0170 force-pushed the 0xc0170:fix_pr_template_required branch from c529b29 to 668f4d3 Feb 27, 2018

@geky

geky approved these changes Feb 27, 2018

@adbridge

LGTM

@sg-

sg- approved these changes Feb 27, 2018

<!--
Required
Please tick one of the following types

This comment has been minimized.

@cmonr

cmonr Feb 27, 2018

Contributor

With the additions of New target and Breaking change should this read "Please tick only one of the following" or "Please tick at least one of the following"?

This comment has been minimized.

@cmonr

This comment has been minimized.

@0xc0170

0xc0170 Feb 27, 2018

Member

Only sounds fine. It should be one of these , not multiple selection here (if it is, PR should be split, doing multiple jobs).

This comment has been minimized.

@adbridge

adbridge Feb 28, 2018

Contributor

I think the wording there is fine, please tick one , means just tick one :)

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 27, 2018

@geky What was the rationale for having Breaking Change again? To me, any of the items in the list could be considered a breaking change.

Would you happen to have an example of what could be a breaking change, but not any of the other items? Or is the intention to have multiple checkmarks (ie: breaking change + fix)?

@geky

This comment has been minimized.

Member

geky commented Feb 27, 2018

Technically speaking, a "breaking change" is a break from documented behaviour. Although you could argue a significant change from "expected" behaivour is also a breaking change. Deciding what is a "breaking change" may require some thinking.

Increasing the size of mbed-os isn't necessarily a breaking change, since we don't document a maximum size, but suddenly increasing mbed-os's footprint to several hundred megabytes would be a breaking change, if only because then it couldn't fit on the platforms we support.

Features should not break documented behaviour. Changing directory iteration to skip every other file would be a "breaking change", but adding a new dir_read_skip_every_other_file function would just be a "feature addition".

It's mostly a matter of severity. The "breaking change" label should be a big red flag that the pr shouldn't be merged without a really good reason.

In theory "breaking changes" only get merged on major releases (not minor ones). However, our current rate of growth + distance between major releases has required some breaking changes in the past to be able to keep moving forward. As the code base becomes more mature breaking changes should become unacceptable outside of major releases.


Side note, a breaking change on a feature before it's made it to a minor release is a bit different, since it hasn't really been released yet. Not sure if those should also be tagged "breaking change" or just "feature" in that case.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Feb 28, 2018

Breaking change is simple - it is an API change, not related to a new feature...

@adbridge

This comment has been minimized.

Contributor

adbridge commented Feb 28, 2018

/morph build

@adbridge adbridge added needs: CI and removed needs: review labels Feb 28, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 28, 2018

Build : SUCCESS

Build number : 1298
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6231/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 28, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 28, 2018

Gotta love those ARM license server issues...

/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr added ready for merge and removed needs: CI labels Feb 28, 2018

@adbridge adbridge merged commit 2b0f169 into ARMmbed:master Mar 1, 2018

17 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs/ Local docs testing has passed
Details
travis-ci/events/ Local events testing has passed
Details
travis-ci/littlefs/ Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL/ Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM/ Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC/ Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON/ Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP/ Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-SILICON_LABS/ Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM/ Local mbed2-STM testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment