workflow: add PR types explanation #475
Conversation
|
|
||
| Any change that results in breaking user space. It should have strong justification to be considered. Often the change could be made backward compatible, for example deprecate the old functionality and introduce the new replacement. | ||
|
|
||
| Release: minor version |
There was a problem hiding this comment.
Shouldn't this be major version?
There was a problem hiding this comment.
Nope, major version is the 5. , next one will be 6. in a year's time
There was a problem hiding this comment.
major would be 6.x , we do 5.x - minor.. .sometimes called feature release.
@AnotherButler what shall we define here ?
Same question below by @geky , should be documented somewhere
There was a problem hiding this comment.
But we don't make breaking changes on minor releases.
(If we do those should be painful exceptions)
Looking at the current rules, what's the motivation for a contributor to make a change "feature" vs "breaking"? They both get released at the same time.
There was a problem hiding this comment.
It is just a classification. If someone rewrites an algorithm which totally changes the behaviour of the system, then that is a breaking change but not a new feature....
There was a problem hiding this comment.
But we don't make breaking changes on minor releases.
Yes, this should not be seen often and thus should really provide strong reason to be considered. there are usually ways to do it backward compatible, but might not always be the case.
It is just a classification. If someone rewrites an algorithm which totally changes the behaviour of the system, then that is a breaking change but not a new feature....
Proposals for better specification of a breaking change ? It can be fix, feature or refactor if it results in breaking a user space. we can provide better examples here?
There was a problem hiding this comment.
A breaking changes is any change, other than the introduction of a new feature, which changes the user experience of the functionality.
|
|
||
| A bug fix should be backward-compatible internal change that fixes incorrect behavior. The fix should provide a description what and how it fixes the issue. The test should be included to catch this issue in the future and confirm the correct behavior. | ||
|
|
||
| Release: patch version |
There was a problem hiding this comment.
Do we go over versions somewhere in docs? maybe we should link there in this page so these terms are defined? @AnotherButler?
|
I'll add the versioning and update this |
|
|
||
| ##### Fix | ||
|
|
||
| A bug fix should be backward-compatible internal change that fixes incorrect behavior. The fix should provide a description what and how it fixes the issue. The test should be included to catch this issue in the future and confirm the correct behavior. |
There was a problem hiding this comment.
Replace "The test should...." with either "A test should..." or "Tests should..."
|
|
||
| A bug fix should be backward-compatible internal change that fixes incorrect behavior. The fix should provide a description what and how it fixes the issue. The test should be included to catch this issue in the future and confirm the correct behavior. | ||
|
|
||
| Release: patch version |
|
|
||
| ##### Fix | ||
|
|
||
| A bug fix should be backward-compatible internal change that fixes incorrect behavior. The fix should provide a description what and how it fixes the issue. The test should be included to catch this issue in the future and confirm the correct behavior. |
There was a problem hiding this comment.
A seperate suggestion: replace suggestive text with active text.
An example: "A bug fix should be backwards-compatible..." would be transformed to "A big fix is backwards-compatible....".
Imo, this makes our enforcement job easier by making reviews stricter, and leaving negotiation leverage with us instead of suggesting that PR authors can barter for leeway.
|
|
||
| ##### Refactor | ||
|
|
||
| Refactors are intended for minor versions if they are not fixing specific issue as they |
There was a problem hiding this comment.
Is it assumed that a refactor is common knowledge here? This described what they're targeted for, but not -what- they are.
|
|
||
| ##### Feature | ||
|
|
||
| New features targets minor releases. It can be integrated only if the feature supports most of the targets (if it requires new target HAL implementation). |
There was a problem hiding this comment.
Echoing @geky comment about what is a major/minor/patch release, if it isn't already documented.
|
|
||
| New features targets minor releases. It can be integrated only if the feature supports most of the targets (if it requires new target HAL implementation). | ||
|
|
||
| Adding a new functionality is considered to be a feature. It does not matter if it is C++ or C. |
There was a problem hiding this comment.
What about how the PR relates to tools? Python?
|
Updated. It now contains versioning section + responded to all @cmonr comments. |
|
|
||
| Any change that results in breaking user space. It should have strong justification to be considered. Often the change could be made backward compatible, for example deprecate the old functionality and introduce the new replacement. | ||
|
|
||
| Release: feature |
There was a problem hiding this comment.
As I added versioning here, this should be major version to be correct.
There was a problem hiding this comment.
I don't believe mbed OS 5 has ever attempted to truly follow semantic versioning, as mbed OS 3 components did - there's been some flexibility on this one. My understanding is we don't deliberately totally break stuff, like binning complete APIs, but we do allow incompatible changes at 5.X to correct API flaws if the deprecation alternative isn't straightforward. (Eg errno mapping changes).
My understanding was that there was an expectation that apps may need to make some changes for a feature release, and as such inclusion version references should be explicit, rather than automatically "take latest" like Yotta, but changes for 5.X should be straightforward.
We're a bit Linux kernely with version number in that regard, rather than user-space shared library. The patch boundary is relatively clear, but when to increment the top digit less so. I have a suspicion the top digit is reserved for marketing.
There was a problem hiding this comment.
I have a suspicion the top digit is reserved for marketing.
ಠ_ಠ
There was a problem hiding this comment.
That sounds correct. I don't think we do our major releases quick enough to strictly enforce semanic versioning, specifically in regards to deprecation warnings.
There was a problem hiding this comment.
@kjbracey-arm Agree. We were flexible (we keep backward compatibility as long as we can, but some breaking changes were done and were part of feature releases).
What goes to major release then?
There was a problem hiding this comment.
From discussions with @sg-, it would be the time to remove /all/ the already-deprecated things, and potentially introduce significant breaking changes that we might be reluctant to do for a regular 5.X.
Wider-scale naming clean-up might also be a thing - some discussions about proposed Serial/UARTSerial structural cleanups have come into that area.
So I'd say major release is very-breaking changes, including refactors that are a bit too breaking to justify for a 5.X, and dropping deprecated stuff.
There was a problem hiding this comment.
Fixed in the new commit (major to be).
| Version number MAJOR.FEATURE.PATCH where: | ||
|
|
||
| - MAJOR version for incompatible API changes | ||
| - FEATURE version for adding functionality in backward-compatible manner |
There was a problem hiding this comment.
Interesting. So we're abandoning the MINOR release terminology?
There was a problem hiding this comment.
"functionality in backward-compatible manner" to "functionality in a backwards-compatible manner"
There was a problem hiding this comment.
Yes, we have feature releases.
language will fix (waiting for Amanda to guidance)
There was a problem hiding this comment.
One of my least favorite "we renamed a standard thing" instances.
|
|
||
| ##### Fix | ||
|
|
||
| A bug fix is backward-compatible internal change that fixes incorrect behavior. The fix provides a description what and how it fixes the issue. Tests are included to catch the issue in the future and confirms the correct behavior. |
There was a problem hiding this comment.
"is backward-compatible" to "is a backwards-compatible"
There was a problem hiding this comment.
"The fix provides a description what and how it fixes the issue."
I'm not sure what's trying to be said here.
There was a problem hiding this comment.
I'm not sure what's trying to be said here.
I'll remove that one, it is in git guidance above
"is backward-compatible" to "is a backwards-compatible"
@AnotherButler I believe both are correct, aren't they? If not, let me know and I'll do edits
|
|
||
| ##### Refactor | ||
|
|
||
| Refactors are intended for feature releases if they are not fixing specific issue as they |
There was a problem hiding this comment.
Is code cleanup considered a refactor?
There was a problem hiding this comment.
Also, is the newline between 102 and 103 intended?
There was a problem hiding this comment.
Is code cleanup considered a refactor?
It really depends on the scope and changes made.
|
|
||
| New features targets feature releases. It can be integrated only if the feature supports most of the targets (if it requires new target HAL implementation). | ||
|
|
||
| Adding a new functionality is considered to be a feature. It does not matter if it is C++, C or Python. |
|
|
||
| Any change that results in breaking user space. It should have strong justification to be considered. Often the change could be made backward compatible, for example deprecate the old functionality and introduce the new replacement. | ||
|
|
||
| Release: feature |
There was a problem hiding this comment.
That sounds correct. I don't think we do our major releases quick enough to strictly enforce semanic versioning, specifically in regards to deprecation warnings.
Every PR type should be clear to a user. It is a user responsibility to specify PR type and accept the versioning based on it. Fixes ARMmbed#429
|
I rebased to resolve conflicts, up to date! |
|
|
||
| ##### Breaking change | ||
|
|
||
| Any change that results in breaking user space. It should have strong justification to be considered. Often the change could be made backward compatible, for example deprecate the old functionality and introduce the new replacement. |
There was a problem hiding this comment.
@SenRamakri asked good question. As this is quite serious type of pull request, we might want to clarify more.
Shall I add breaking user space (an application requires changes to continue working) or similar. This also leads to another good question is - what can actually break (what is considered public vs internal) ? It might be good to answer this via separate PR. So lets stick here for what is breaking change and how to make it clear to users.
|
Another round for review ! 📲 |
Copy edit latest content.
|
We have most/some of this content here: https://os-doc-builder.test.mbed.com/docs/development/introduction/how-we-release-arm-mbed-os.html Would it make sense to move that content from the intro to here? |
I can move there the versioning. And reference it here? Need to find where is md file for that page |
|
@AnotherButler I updated the PR. As the document you shared with me already contains the info I was looking for, I kept the section but just a reference to the release document (this way people can get versioning info that we reference below). Review please |
Fix sentence for consistent voice throughout and across documents
|
@sg- Is this OK to merge, or does your "needs changes" still stand? |
Make changes from PR #475 to live site after reviewing on test site.
Every PR type should be clear to a user.
It is a user responsibility to specify PR type and accept the versioning based on it.
Fixes #429
Proposal to define PR types. I would like to reference this in every pull request so all of us are aware of the responsibility to select the proper type and the release label selection based on that.
Needs more work to be more clear and specify more details, expecting lot of comments 🔥
@sg- @cmonr @adbridge @theotherjimmy @kjbracey-arm Please review