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

Conversation

reyang
Copy link
Member

@reyang reyang commented Jul 27, 2020

Changes

This was discussed in the 7/27/2020 specification SIG meeting.
The goal is to have a better clarity on how to make progress on PRs.

@reyang reyang requested a review from a team July 27, 2020 21:20
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@reyang reyang added the area:miscellaneous For issues that don't match any other area label label Jul 28, 2020
CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

LGTM.
Left couple of minor, non blocking comments.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
reyang and others added 4 commits July 28, 2020 07:56
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
CONTRIBUTING.md Outdated Show resolved Hide resolved
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
@carlosalberto
Copy link
Contributor

what percentage of their lifetime do they spend waiting for a maintainer to merge them after having enough approvals?

I don't think that happens that much. What happens sometimes is that a maintainer or approver requests changes, hence blocking the PR.

@anuraaga
Copy link
Contributor

I don't think that happens that much.

The recent official triage during SIG has sped things up a lot from what I've seen, but I also worry about the scalability of it. This is so recent though that the current best practice for my team is still, "to get a PR merged, join the SIG meeting" - I hope this can go away as the new triage process shows fruit, but I wouldn't be so optimistic about this quite yet.

@carlosalberto
Copy link
Contributor

@anuraaga I disagree. Most PRs with enough approvals have been merged properly. The problem is mostly with unsolved issues or lack of reviews, which indeed sadly forces authors to join the SIG call to ask for attention (on which we still need to do a better job).

CONTRIBUTING.md Show resolved Hide resolved
owners](./.github/CODEOWNERS) (if approvals are from only one company, they
won't count).
* There is no `request changes` from the [code owners](./.github/CODEOWNERS).
* It has been at least two working days since the last modification (except for
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)

@anuraaga
Copy link
Contributor

I was reminded of this again after seeing

#606

Sorry to say it again but I'm a bit surprised maintainers would contend that PRs get merged properly. I have seen too many times they get stuck because approvers / maintainers don't take charge and get them through. Pinging a reviewer for more input is the easiest way to kill a PR currently since often that input doesn't come.

So I propose this PR doesn't go far enough - it still puts too much burdern on the contributor, the language even seems to lean towards "the contributor is probably wrong". It's the maintainer's responsibility to move things along (either yes or no, the result is not as important). For this to be a community, I think we need process here that puts more responsibility on maintainers. BDFL model makes this the easiest, but lacking that we need something that can still come close or contributors will just go away when they try something but no one proactively helps make it happen.

@carlosalberto
Copy link
Contributor

it still puts too much burdern on the contributor, the language even seems to lean towards "the contributor is probably wrong". It's the maintainer's responsibility to move things along (either yes or no, the result is not as important). For this to be a community, I think we need process here that puts more responsibility on maintainers.

I disagree partially - for sure, maintainers need to be more proactive on handling (stale) issues and PRs, but we expect OTel organization members/approvers to put their own cycles to move their issues/PRs forward. For example: it's very common that the author gets busy and forgets about the PR (I can already see a few of them currently open), or they simply forget to provide feedback.

For external contributors that's something else - but we can worry about that once we hit GA (I'm pretty sure we will get a lot of external users feedback/requests changes).

@carlosalberto carlosalberto self-requested a review August 3, 2020 13:00
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Blocking this PR since it is not the right place to change community membership decisions.

@Oberon00
Copy link
Member

Oberon00 commented Aug 4, 2020

@bogdandrutu Where would be the right place to discuss this? I think TC approval and a community repository PR will certainly be needed, but I believe that discussing this (also) here is best for visibility.

@bogdandrutu
Copy link
Member

I think the right place is in public in a community issue, because the gouvarnance is responsible to define that role

@bogdandrutu
Copy link
Member

bogdandrutu commented Aug 4, 2020

I know I wear two hats here but would prefer other members of the GC to have visibility into this, and they usually don't follow this repo

@carlosalberto
Copy link
Contributor

Let's just remove the 'offending' part and discuss it somewhere else (most likely as an issue the community repo, as outlined by @bogdandrutu ) so we can move forward this PR.

CONTRIBUTING.md Outdated Show resolved Hide resolved
merge the PR once it is **ready to merge**.

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:
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:

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

* It has received more than two approvals from the [code
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.

Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
@carlosalberto
Copy link
Contributor

We were meant to merge this last week, but as they say: better late than never!

@carlosalberto carlosalberto merged commit 98bc818 into open-telemetry:master Aug 6, 2020
@reyang reyang deleted the reyang/pr-process branch October 4, 2021 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:miscellaneous For issues that don't match any other area label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants