Skip to content

Workflow document update #377

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
merged 1 commit into from
Feb 22, 2018
Merged

Workflow document update #377

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
111 changes: 66 additions & 45 deletions docs/reference/contributing/guidelines/workflow.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,45 +1,52 @@
### Workflow

#### Contributions

All code changes and additions to Mbed OS are handled through GitHub. If you want to contribute, either by adding features or by fixing bugs, please follow the guidelines for [new features](#contributing-new-features-to-mbed-os) and [bugs](#reporting-and-fixing-bugs). In both cases, please follow the [code style guide and GitHub pull request guidelines](/docs/v5.7/reference/guidelines.html#style)</a>. Please also read the [CLA](/docs/v5.7/reference/guidelines.html#cla) guidelines because we will immediately close pull requests submitted without a CLA.

#### Contributing new features to Mbed OS
All code changes and additions to Mbed OS are handled through GitHub. If you want to contribute, either by adding features or by fixing bugs, please follow the guidelines for [new features](#features) and [bugs](#reporting-bugs). In both cases, please follow the [code style guide and GitHub pull request guidelines](/docs/v5.7/reference/guidelines.html#style). Please also read the [Contributor License Agreement (CLA)](/docs/v5.7/reference/guidelines.html#cla) guidelines because we will immediately close pull requests submitted without a CLA.

Before contributing an enhancement (new feature, new port and so on), please [discuss it on the forums](https://os.mbed.com/forum/bugs-suggestions/) to avoid duplication of work, as we or others might be working on a related feature.
#### Mbed OS maintainers

Patch contributions can only be accepted through GitHub by creating a pull request from forked versions of our repositories. This allows us to review the contributions in a user friendly and reliable way, under public scrutiny.
The maintainers are a small group of Mbed OS engineers who are responsible for the Mbed OS codebase. Their primary role is to progress contributions, both internal and external, from the initial pull request state through to released code.

Please create separate pull requests for each concern; each pull request should have a clear unity of purpose. In particular, separate code formatting and style changes from functional changes. This makes each patch’s true contribution clearer and therefore quicker and easier to review.
Responsibilities:

#### Reporting and fixing bugs
1. Check for CLA compliance.
1. Ensure the relevant stakeholders review pull requests.
1. Guide contributors both technically and procedurally.
1. Run pull requests through the CI systems.
1. Put label version.
Copy link
Member

Choose a reason for hiding this comment

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

That probably doesn't mean much to an external reader.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a description of our process, this documentation is for internal use as well as external

Copy link
Member

Choose a reason for hiding this comment

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

Exactly, I would say anyone reading it should be able to understand what's the process (external or newstarter).

1. Merge pull requests into the requested branches.
1. Make periodic patch and feature releases.

Before submitting a bug report or a bug fix, please [discuss it on the forums](https://os.mbed.com/forum/bugs-suggestions/) to avoid duplication of work, as we or others might be working on it already.
The current maintainers are:

##### Bug reports (issues) on GitHub
* [Anna Bridge](https://os.mbed.com/users/AnnaBridge).
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be easier to get an alias/group and not have to maintain that list?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if they can do that on the os.mbed.com site...

Copy link
Contributor Author

@0xc0170 0xc0170 Feb 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We will have a group soon (not on mbed.os though). Once done, we can update it separately. This will come in the following days

* [Martin Kojtal](https://os.mbed.com/users/Kojto).
* [Jimmy Brisson](https://os.mbed.com/users/theotherjimmy).
* [Shrikant Tudavekar](https://os.mbed.com/users/shrikant1213).
* [Sam Grove](https://os.mbed.com/users/sam_grove).
* [Cruz Monrreal](https://os.mbed.com/users/MrCruz).

All Mbed OS is on GitHub; please use GitHub's [issues mechanism](https://guides.github.com/features/issues/) to open a bug report directly against the relevant GitHub repository.
#### Contributions

##### Bug fixes
Before contributing an enhancement (new feature, new port and so on), please [discuss it on the forums](https://os.mbed.com/forum/bugs-suggestions/) to avoid duplication of work, as we or others might be working on a related feature.

Please refer to the [code contributions chapter](/docs/v5.7/reference/guidelines.html#style).
We can only accept contributions through GitHub if you create a pull request from forked versions of our repositories. This allows us to review the contributions in an easy-to-use and reliable way, under public scrutiny.

A member of the Mbed team must verify bug fixes before we pull the fixes into the main branch. You must therefore use GitHub to fork the repo and then submit a pull request with your changes.
Please create separate pull requests for each concern; each pull request needs a clear unity of purpose. In particular, separate code formatting and style changes from functional changes. This makes each pull request’s true contribution clearer, so review is quicker and easier.

The last line in your commit message description should say “Fixes #deadbeef”, where “deadbeef” is the issue number in GitHub. This allows GitHub to automatically close the issue when the commit is merged into the default branch.
##### Reporting bugs

#### Further reading
Please submit all Mbed OS bugs [on the forums](https://os.mbed.com/forum/bugs-suggestions/).
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this right? Are we now asking people to do this rather than directly raise issues? Is this so the support team can pick these up and raise the issues themselves ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correct, more update will come from support team


Please see the [code contributions chapter](/docs/v5.7/reference/guidelines.html#style) for the guidelines to GitHub pull requests and the coding style guide.
The bug report should be reproducible (fails for others) and specific (where and how it fails). We will close insufficient bug reports.
Copy link
Member

Choose a reason for hiding this comment

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

The bug report should be reproducible (fails for others)

If it's an intermittent issue it may not be readily reproducible.

Should we put more details what we expect to be in a report? Like versions, steps, example, expected and actual outcomes,...

Copy link
Contributor

Choose a reason for hiding this comment

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

There should be enough information captured so that anyone trying to reproduce the bug can replicate the environment and code versions used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we put more details what we expect to be in a report? Like versions, steps, example, expected and actual outcomes,...

Having how to report a bug would be nice to have, currently there is something in github template issue (not sufficient though). I'll talk to the team who will be handling issues


#### Guidelines for GitHub pull requests

Pull requests on GitHub have to meet the following requirements to keep the code and commit history clean:

- Commits should always contain a proper description of their content. Start with a concise and sensible one-line description. Then, elaborate on reasoning of the choices made, descriptions for reviewers and other information that might otherwise be lost.
- Commits should always contain a proper description of their content. Start with a concise and sensible one-line description. Then, elaborate on reasoning of the choices made, descriptions for reviewers and other information that might otherwise be lost.
- You should always write commits to allow publication, so they can never contain confidential information, reference private documents, links to intranet locations or rude language.
- Each commit should be the minimum self-contained commit for a change. A commit should always result in a new state that is again in a compilable state. You should (if possible) split large changes into logical smaller commits that help reviewers follow the reasoning behind the full change.
- Commits should follow [Chris Beam’s seven rules of great commit messages](http://chris.beams.io/posts/git-commit#seven-rules):
- Commits and pull request titles should follow [Chris Beam’s seven rules of great commit messages](http://chris.beams.io/posts/git-commit#seven-rules):
1. Separate subject from body with a blank line.
1. Limit the subject line to 72 characters (note that this is a deviation from Beam's standard).
1. Capitalize the subject line.
Expand All @@ -50,6 +57,27 @@ Pull requests on GitHub have to meet the following requirements to keep the code
- Because we use GitHub and explicit CLAs, special commit tags that other projects may use, such as “Reviewed-by”, or “Signed-off-by”, are redundant and should be omitted. GitHub tracks who reviewed what and when, and our stack of signed CLAs shows us who has agreed to our development contribution agreement.
- Prefixing your commit message with a domain is acceptable, and we recommend doing so when it makes sense. However, prefixing one's domain with the name of the repo is not useful. For example, making a commit entitled "mbed-drivers: Fix doppelwidget frobulation" to the `mbed-drivers` repo is not acceptable because it is already understood that the commit applies to `mbed-drivers`. Renaming the commit to "doppelwidget: Fix frobulation" would be better, if we presume that "doppelwidget" is a meaningful domain for changes, because it communicates that the change applies to the doppelwidget area of `mbed-drivers`.
- All new features and enhancements require documentation, tests and user guides for us to accept them. Please link each pull request to all relevant documentation and testing pull requests.
- Avoid merging commmits. (Always rebase when possible.)
- Pull requests should fix a bug, add a feature or refactor.

#### Pull request categories

##### Bug fixes

Every bug fix should contain a test to verify results prior to and after the pull request.

##### Changes and additions

Backward compatible changes (such as refactoring and enhancements) or new target additions can go into patch and feature releases. We only consider features and new functionality additions for feature releases.
Copy link
Member

Choose a reason for hiding this comment

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

We should describe something what a patch or feature release is. Also I'm not sure what a feature release is we should probably just stick to versions nomenclature: major, minor, patch.

Copy link
Contributor

Choose a reason for hiding this comment

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

We started off with semantic nomenclature but were told by the powers that be to use feature and patch instead


##### Features

We initially implement new features on separate branches in the Mbed OS repository. Mbed OS maintainers create the new branches by following the naming convention: "feature-" prefix.

Each feature has a tech lead. This person is responsible for:

- Rebasing often to track master development.
- Reviewing any addition to the feature branch (approval required by the feature tech lead or another assigned person).

##### Pull request template

Expand All @@ -75,52 +103,45 @@ You can see test results [here](just an example).

```

#### Mbed OS maintainers

The maintainers are a small group of Mbed OS engineers who are responsible for the Mbed OS codebase. Their primary role is to progress contributions, both internal and external, from the initial pull request state through to released code. They:

1. Check for CLA compliance.
2. Ensure the relevant stakeholders review pull requests.
3. Guide contributors both technically and procedurally.
4. Run pull requests through the CI systems.
5. Merge pull requests into the requested branches.
6. Make periodic patch and feature releases.

The current maintainers are:

* Anna Bridge (adbridge).
* Martin Kojtal (0xc0170).
* Jimmy Brisson (theotherjimmy).
* Shrikant Tudavekar (studavekar).
* Sam Grove (sg-).

#### GitHub pull requests workflow

Each pull request goes through the following workflow:

<span class="images">![](https://s3-us-west-2.amazonaws.com/mbed-os-docs-images/Workflow.png)<span>The workflow of merging a pull request</span></span>

##### Pull request states
#### Pull request states

Labels that the Mbed OS maintainers add to a pull request represent the pull request workflow states. The Mbed OS maintainers are responsible for moving pull requests through the workflow states.
The Mbed OS maintainers add labels to a pull request that represent the pull request workflow states. The Mbed OS maintainers are responsible for moving pull requests through the workflow states.

Each state is time boxed. In most cases, this is sufficient time to move to another state. The pull request can be closed if no update is provided within the time frame.
Copy link
Member

Choose a reason for hiding this comment

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

In most cases, this is sufficient time to move to another state.

Not sure what does it mean

The pull request can be closed if no update is provided within the time frame.

Should we either define it or reword it to something like "The pull request can be closed if it's not actively maintained"

Copy link
Contributor

Choose a reason for hiding this comment

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

Another state in the workflow. ie go from needs review to needs ci.

We need some kind of time frame that people are aware of . If it is ambiguous and we start randomly closing PRs then people will complain. If it is a clear cut rule then there can be no complaints. After all, the time limits should be for responses not necessarily to completely fix something. This is to prevent open PRs just sitting there with no response from the authors for weeks at a time...


##### Reviews

All pull requests must be reviewed. The Mbed OS maintainers determine the most suitable person to review the pull request and tag that person accordingly. If a pull request requires a review from partners, the maintainers tag the corresponding GitHub group instead.
All pull requests must be reviewed. The Arm Mbed CI bot determines the most suitable person to review the pull request and tags that person accordingly.

Time: 3 days for reviewers to leave feedback after the maintainers add the "needs: review" label.
Copy link
Contributor

Choose a reason for hiding this comment

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

As this is going to be the bot adding this label we should re-word this to:

Time: 3 days for reviewers to leave feedback once the "needs: review" label has been applied.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to do an update to this once bot is implemented and ready for action (thus it is manual at the moment)

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't like us to commit to 3 days, we may have other things on our plate and big, unrelated PR may not be a priority. I'm trying to go through my github feed twice a week, but doesn't always happen. Unless it's a guidance not a deadline.


##### The CI (Continuous Integration) testing

There are a number of CI systems available. Which CI tests we run against a particular pull request depends on the effect of that pull request to the code base. Irrespective of which CIs tests run, Mbed OS has an all green policy, meaning that all the CI jobs that are triggered must pass before we merge the pull request.
There are many CI systems available. Which CI tests we run against a particular pull request depends on the effect that pull request has on the code base. Irrespective of which CIs tests run, Mbed OS has an all green policy, meaning that all the CI jobs that are triggered must pass before we merge the pull request.

Time: 1 day for CI to complete and report back results.

##### Work needed

A pull request in the work needed state requires additional work due to failed tests or rework as a result of the review. If a pull request is in this state, our maintainers request changes from the pull request author.

Time: 3 days for the pull request author to action the review comments.

##### Releases

Once we merge a pull request, we tag it with a release. This is the release in which we first publish this pull request. For patch releases, we allow only bug fixes, new targets and enhancements to existing functionality. New features only go to feature releases.
When we merge a pull request that we will publish in a patch release, we tag the pull request with the specific patch release version. This is the release in which we first publish this pull request. For patch releases, we allow only bug fixes, new targets and enhancements to existing functionality. New features only go to feature releases.

The release tag has the following format:

*release-version: 5.f.p* - Where `f` is the feature release and `p` the patch release.

##### Additional labels
#### Additional labels

We use many other labels to summarize the scope and effect of the changes.

Expand Down