Update workflow document to include mention of rollup PRs #801
Conversation
| @AnotherButler @ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-test Please review. |
| | ||
| #### Rollup Pull Requests | ||
| | ||
| At times, Mbed OS will contain many small pull requests that are either unrelated to each other (completely different tests or targets), or have a low risk of failing CI testing (docs changes). At the discretion of a maintainer, a pull request will be additionally tagged with the *rollup PR* label and a script will create a rollup pull request to take through testing. Upon success, the rollup pull request is merged as normal, and all pull requests that were marked with the *rollup PR* label will be marked as merged. |
There was a problem hiding this comment.
nitpicking: does Mbed OS contain PRs ?
I though it was something like PRs are raised against MbedOS
There was a problem hiding this comment.
Yes. Sounds strange to say something contains PRs.
| | ||
| #### Rollup Pull Requests | ||
| | ||
| At times, Mbed OS will contain many small pull requests that are either unrelated to each other (completely different tests or targets), or have a low risk of failing CI testing (docs changes). At the discretion of a maintainer, a pull request will be additionally tagged with the *rollup PR* label and a script will create a rollup pull request to take through testing. Upon success, the rollup pull request is merged as normal, and all pull requests that were marked with the *rollup PR* label will be marked as merged. |
There was a problem hiding this comment.
(completely different tests or targets): does this include PRs that modify different OS modules but have nothing to do with targets or tests?
There was a problem hiding this comment.
I think in this context, yes. Will reword.
| - *DO NOT MERGE* - This pull request contains changes that may be in a draft state and submitted purely for review, or may contain breaking changes that have not been considered. | ||
| - *devices: 'name'* - The pull request specifically affects the named device(s). | ||
| - *component: 'name'* - The pull request specifically affects the named component. Component names follow the structure of Mbed OS (`ble`, `storage`, `tls` and so on). | ||
| - *rollup PR* - This pull request is ready to begin CI testing, but will be tested with other pull requests at once. |
There was a problem hiding this comment.
in terms of tag semantics, with this tag be used in addition to "needs CI" or replace it in the case of rollup PRs?
There was a problem hiding this comment.
Italics don't stand out very well. Recommend using bold instead.
There was a problem hiding this comment.
Italics don't stand out very well. Recommend using bold instead.
@melwee01 Intention was to follow styling already present. Should I update the remaining tag labels as well?
There was a problem hiding this comment.
in terms of tag semantics, with this tag be used in addition to "needs CI" or replace it in the case of rollup PRs?
The original intent was to have the tag live next to a "needs: CI" label, to lessen any complications that might arise from adding another state. In addition, there's no technical reason why we couldn't have multiple rollup PRs active at once, in parallel. With a seperate label, we could migrate to somethink like "rollup: A", "rollup: B", "rollup: C", but that's me.
That being said, I could also imagine where "needs: CI" would transition to "needs: rollup" or "in: rollup".
| | ||
| #### Rollup Pull Requests | ||
| | ||
| At times, Mbed OS will contain many small pull requests that are either unrelated to each other (completely different tests or targets), or have a low risk of failing CI testing (docs changes). At the discretion of a maintainer, a pull request will be additionally tagged with the *rollup PR* label and a script will create a rollup pull request to take through testing. Upon success, the rollup pull request is merged as normal, and all pull requests that were marked with the *rollup PR* label will be marked as merged. |
There was a problem hiding this comment.
really nitpicking: "Upon success, the rollup pull request is merged as normal" - when you say this it may not be clear to the reader if by "rollup pull request" you are referring to the individual PR marked with the rollup tag, or the PR combining all the rollup labled PRs. maybe it may help to define some basic terminology first
| | ||
| At times, Mbed OS will contain many small pull requests that are either unrelated to each other (completely different tests or targets), or have a low risk of failing CI testing (docs changes). At the discretion of a maintainer, a pull request will be additionally tagged with the *rollup PR* label and a script will create a rollup pull request to take through testing. Upon success, the rollup pull request is merged as normal, and all pull requests that were marked with the *rollup PR* label will be marked as merged. | ||
| | ||
| No additional work from a pull request author is needed. If the rollup pull request fails testing, maintainers will root cause the issue, and reach out to the author(s) of the problematic pull request(s) that a problem was identified. Optionally, a maintainer may choose to regenerate the rollup pull request without the problem pull request(s). |
There was a problem hiding this comment.
this sentence looks too complex to me (I understand what you are saying because I'm a bit familiar with the material, but others might not ), maybe simplify? :
If the rollup pull request fails testing, maintainers will root cause the issue, and reach out to the author(s) of the problematic pull request(s) that a problem was identified.
| | ||
| At times, Mbed OS will contain many small pull requests that are either unrelated to each other (completely different tests or targets), or have a low risk of failing CI testing (docs changes). At the discretion of a maintainer, a pull request will be additionally tagged with the *rollup PR* label and a script will create a rollup pull request to take through testing. Upon success, the rollup pull request is merged as normal, and all pull requests that were marked with the *rollup PR* label will be marked as merged. | ||
| | ||
| No additional work from a pull request author is needed. If the rollup pull request fails testing, maintainers will root cause the issue, and reach out to the author(s) of the problematic pull request(s) that a problem was identified. Optionally, a maintainer may choose to regenerate the rollup pull request without the problem pull request(s). |
There was a problem hiding this comment.
"Optionally, a maintainer may choose to regenerate the rollup pull request without the problem pull request(s). "
here I'd add a note that if a PR is removed from the rollup for whatever reason, the label will be removed and it will be returned to regular "Needs CI" or "Needs Work" status.
There was a problem hiding this comment.
That's fair. I'll add something about updating it's status.
| | ||
| ##### How it works | ||
| | ||
| Rollup pull requests are generated by creating a branch off of the HEAD of master. Once the branch is created, pull requests are scanned for the *rollup PR* label, and each pull request that is found to have the label, the same commands that would be used to merge a pull request into master are instead performed into the rollup branch. A pull request is then opened normally against master using this new branch. |
There was a problem hiding this comment.
this is only explains how the rollup branch is created, not what happens later (testing / merging the changes).
It might be nice to add a bit of details on how the merge part of it works (PRs will appear as though they were merged normally in the order they were added to the rollup IIRC).
There was a problem hiding this comment.
each pull request that is found to have with the label
There was a problem hiding this comment.
and each pull request that is found to have the label, the same commands that would be used to merge a pull request into master are instead performed into the rollup branch.
There's non-parallel construction here.
There was a problem hiding this comment.
I will add more, but I wanted to avoid adding too much detail, less the purpose of the page get diluted.
Will work on my sentence structure(s).
There was a problem hiding this comment.
I'll add more. I was concerned about adding too much details in what I view is an overview page.
| @AnotherButler @melwee01 This is ready for your review |
There was a problem hiding this comment.
Thanks @cmonr , looks much better. a few more minor comments (sorry for the nagging).
| | ||
| #### Rollup pull requests | ||
| | ||
| When Mbed OS has many small, orthogonal pull requests waiting for CI testing to start, maintainers have the option of putting multiple pull requests through CI by creating a rollup pull request. Rollup pull requests are generated by merging other pull requests into a temporary branch, and opening a new pull requests against this new branch. Once the rollup pull request passes CI testing and is merged, the original pull requests that were used to build the rollup pull requests are also automatically merged. |
There was a problem hiding this comment.
a new pull requests => a new pull request ?
| | ||
| When Mbed OS has many small, orthogonal pull requests waiting for CI testing to start, maintainers have the option of putting multiple pull requests through CI by creating a rollup pull request. Rollup pull requests are generated by merging other pull requests into a temporary branch, and opening a new pull requests against this new branch. Once the rollup pull request passes CI testing and is merged, the original pull requests that were used to build the rollup pull requests are also automatically merged. | ||
| | ||
| When a pull request is selected to be integrated into a rollup pull requests, it already has a release label and is patiently waiting to start CI testing. Once selected to be looped into a rollup pull request, the original pull request will gain a *rollup PR* label. When the rollup pull request is generated, a comment will also be added to the original pull request mentioning that no further work needs to be done to it. |
There was a problem hiding this comment.
I was trying to come up with a verb for the PRs that get rolled up. Still dunno what to call it.
There was a problem hiding this comment.
suggestions: rolled-up , combined, integrated, pooled, hooked-up, joined-up, etc... (you can use a thesaurus to find more ideas)
There was a problem hiding this comment.
added to a rollup pull request?
| | ||
| When Mbed OS has many small, orthogonal pull requests waiting for CI testing to start, maintainers have the option of putting multiple pull requests through CI by creating a rollup pull request. Rollup pull requests are generated by merging other pull requests into a temporary branch, and opening a new pull requests against this new branch. Once the rollup pull request passes CI testing and is merged, the original pull requests that were used to build the rollup pull requests are also automatically merged. | ||
| | ||
| When a pull request is selected to be integrated into a rollup pull requests, it already has a release label and is patiently waiting to start CI testing. Once selected to be looped into a rollup pull request, the original pull request will gain a *rollup PR* label. When the rollup pull request is generated, a comment will also be added to the original pull request mentioning that no further work needs to be done to it. |
There was a problem hiding this comment.
"a comment will also be added to the original pull request mentioning that no further work needs to be done to it"
this comment may be confusing, particularly if the PR later needs to be removed from the rollup-PR in which case it will actually require more work. I think what you may want to convey is that while the PR is bein processed as part of the rollup no further work can be done on the original PR (otherwise the rollup will need to be re-generated).
There was a problem hiding this comment.
But that's not actually true. If a PR that was put into a Rollup PR is updated on its own, work shouldn't be needed in the original PR. However, if an additional commit shows up in the original PR, and the Rollup PR completes CI and is merged, the updated PR will remain open because not all commits were in the Rollup PR.
We actually had this happen with one of the early tests of the Rollup PR, and fortulately nothing critical broke :)
| | ||
| When a pull request is selected to be integrated into a rollup pull requests, it already has a release label and is patiently waiting to start CI testing. Once selected to be looped into a rollup pull request, the original pull request will gain a *rollup PR* label. When the rollup pull request is generated, a comment will also be added to the original pull request mentioning that no further work needs to be done to it. | ||
| | ||
| In the event that the rollup pull request fails CI testing, maintainers will identify the offending pull requests, update their statuses, and will provide additional guidance on what went wrong. Additionally, if a pull request is updated *while* it is already in a rollup pull request, and the rollup pull request passes CI and is merged, the pull request that was updated *will not* be automatically merged. |
| | ||
| Rollup pull requests are a solution to two types of problem: CI testing duration, and semantic conflict problem. The most obvious use of a rollup pull request is to drastically lower the time needed to test many pull requests at once. Best case, maintainers go from needing to put N pull requests through CI, down to only one, drastically lowering the load on CI infrastructure, and helps close pull requests much sooner. The second, more suble, problem that rollup pull requests solve is the case where two pull requests pass testing on their own, but as soon as they join together, the way they interact with each other causes tests to fail. A simple example can be found [here](https://bors.tech/essay/2017/02/02/pitch). | ||
| | ||
| When merged, rollup pull requests will automatically cause the pull requests that made it to also be marked as merged as well. The commits that made up the original pull requests are also apart of the rollup pull requests, so when the rollup pull request is merged, the changes in the pull requests are already merged and the pull request is updated accordingly. This is also why if a new change is added to a pull request _after_ the rollup pull request is made, it _will not_ automatically be merged once the rollup pull request is, because all changes the updated pull request are still not in master. |
There was a problem hiding this comment.
Nitpick
"When merged, rollup pull requests will automatically cause the pull requests that made it to also be marked as merged as well" - you repeat this statement in different forms several times in this text (e.g. in the frist paragraph of "How it works", and also in Rollup pull requests ). Maybe you can remove one?
There was a problem hiding this comment.
Removed the first instance, to make the first section smaller and more general.
| @cmonr Can you respond to the review comments, this should be ready |
Hopefully clarified and remove redundances from deeper technical details
| @0xc0170 Responded. Would appreciate a re-review. |
| ping @0xc0170 @NirSonnenschein |
| | ||
| Rollup pull requests are a solution to two types of problem: CI testing duration, and semantic conflict problem. The most obvious use of a rollup pull request is to drastically lower the time needed to test many pull requests at once. Best case, maintainers go from needing to put N pull requests through CI, down to only one, drastically lowering the load on CI infrastructure, and helps close pull requests much sooner. The second, more suble, problem that rollup pull requests solve is the case where two pull requests pass testing on their own, but as soon as they join together, the way they interact with each other causes tests to fail. A simple example can be found [here](https://bors.tech/essay/2017/02/02/pitch). | ||
| | ||
| A special case occurs when a bundled pull request is updated while its rollup pull requests is undergoing testing. When a pull request is bundled into a rollup pull request, it's commits become a part of the rollup pull request, at the time that the rollup pull request source branch is created. If the bundled pull requests is updated, its commit history is no longer exactly mirrored in the rollup pull request. If this situation arises, the bundled pull request that was updated _will not_ be automatically marked as merge, because all changes of the updated bundled pull request were not present in the merged rollup pull request. |
There was a problem hiding this comment.
what should happen if this is the case? No action needed from a user or us?
There was a problem hiding this comment.
Ah, fair question. If this happens, work will continue in that updated, previously bundled PR.
Will update.
…pdated and therefore not merged. Also minor grammar nit
| @0xc0170 Updated. Please rereview |
Edit file, mostly for consistent person and tense.
| Does this still need a content rereview, or is it OK to merge? |
| @ARMmbed/mbed-os-maintainers Last call |
| I'm going to merge this to our development site. If anyone has further changes, please open a new PR. |
7 participants
Added details about additional label that might show up in PRs, along with what a Rollup PR is, and how it works.
Would also like to work in that it's helps solve this problem, but not sure how to word it in.