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 21 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.
73 changes: 73 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,76 @@ To quickly fix typos, use
```bash
make misspell-correction
```

## Pull Requests

### How to create a PR

Everyone is welcome to contribute to the OpenTelemetry specification via GitHub
pull requests (PRs).

To [create a new
PR](https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/creating-a-pull-request),
fork the project in GitHub and clone the upstream repo:

```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.

If the PR is not ready for review, please mark it as
[`draft`](https://github.blog/2019-02-14-introducing-draft-pull-requests/).

Please make sure CLA is signed and CI is clear. We don't expect people to review
and comment on a PR that doesn't have CLA signed.

### How to get your PR merged

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

* It has received more than two approvals from the [code
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
owners](./.github/CODEOWNERS) (if approvals are from only one company, they
Copy link
Member

Choose a reason for hiding this comment

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

This is hard to enforce by GitHub, but I am fine with this in general

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. We can look into this in the medium future.

won't count).
* There is no `request changes` from the [code owners](./.github/CODEOWNERS).
carlosalberto 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 changes (typos, cosmetic changes, CI improvements, etc.) don't have to
wait for two days.

Any [spec
maintainers](https://github.com/open-telemetry/community/blob/master/community-members.md#specifications-and-proto) can
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
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. It is
recommended to add a link into the PR description, which points to a comment
with a summary in the PR conversation.
* 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 narrow down the scope of the PR or
split it up.

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