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

Clarify how to make progress on a PR #745

Merged
merged 22 commits into from
Aug 6, 2020
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
File renamed without changes.
67 changes: 67 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,70 @@ To quickly fix typos, use
```bash
make misspell-correction
```

## Pull Request
reyang marked this conversation as resolved.
Show resolved Hide resolved

### How to send PR
reyang marked this conversation as resolved.
Show resolved Hide resolved

Everyone is welcome to contribute to `OpenTelemetry Specification` via GitHub
reyang marked this conversation as resolved.
Show resolved Hide resolved
pull requests (PRs).

To create a new PR, fork the project in GitHub and clone the upstream repo:
reyang marked this conversation as resolved.
Show resolved Hide resolved

```sh
git clone https://github.com/open-telemetry/opentelemetry-specification.git
```

Add your fork as a remote:

```sh
git remote add fork https://github.com/YOUR_GITHUB_USERNAME/opentelemetry-specification.git
```

Check out a new branch, make modifications and push the branch to your fork:

```sh
$ git checkout -b feature
# edit files
$ git commit
$ git push fork feature
```

Open a pull request against the main `opentelemetry-specification` repo.

### How to receive comments
reyang marked this conversation as resolved.
Show resolved Hide resolved

* If the PR is not ready for review, please put `[WIP]` in the title or mark it
reyang marked this conversation as resolved.
Show resolved Hide resolved
as [`draft`](https://github.blog/2019-02-14-introducing-draft-pull-requests/).
* Make sure CLA is signed and CI is clear.

### How to get PR merged
reyang marked this conversation as resolved.
Show resolved Hide resolved

A PR is considered to be **ready to merge** when:

* It has received two approvals from the [code owners](./.github/CODEOWNERS) (at
reyang marked this conversation as resolved.
Show resolved Hide resolved
different companies).
* Major feedbacks are resolved.
reyang marked this conversation as resolved.
Show resolved Hide resolved
* It has been at least two working days since the last modification (except for
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

I'd also add that any significant change should be clearly mentioned in a separate comment to not go unnoticed.
If the PR turns into a different direction after approving reviews, those should be dismissed and asked for a re-review.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Same as the previous comment)

the trivial updates, such like typo, cosmetic, rebase, etc.). This gives
people reasonable time to review.
* Trivial change (typo, cosmetic, CI improvement, etc.) doesn't have to wait for
reyang marked this conversation as resolved.
Show resolved Hide resolved
two days.

Any [spec
approver](https://github.com/orgs/open-telemetry/teams/specs-approvers) can
merge the PR once it is **ready to merge**.
reyang marked this conversation as resolved.
Show resolved Hide resolved

If a PR has been stuck (e.g. there are lots of debates and people couldn't agree
on each other), the owner should try to get people aligned by:
reyang marked this conversation as resolved.
Show resolved Hide resolved
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
on each other), the owner should try to get people aligned by:
with each other), the owner should try to get people aligned by:


* Consolidating the perspectives and putting a summary in the PR.
reyang marked this conversation as resolved.
Show resolved Hide resolved
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
* Tagging subdomain experts (by looking at the change history) in the PR asking
for suggestion.
* Reaching out to more people on the [gitter
channel](https://gitter.im/open-telemetry/opentelemetry-specification).
* Stepping back to see if it makes sense to scope down the PR.
reyang marked this conversation as resolved.
Show resolved Hide resolved

If none of the above worked and the PR has been stuck for more than 2 weeks, the
owner should bring it to the [OpenTelemetry Specification SIG
meeting](https://github.com/open-telemetry/community#cross-language-specification).
1 change: 1 addition & 0 deletions docfx.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
"resource": [
{
"files": [
".github/CODEOWNERS",
".markdownlint.yaml",
".vscode/settings.json",
"**.png"
Expand Down