Approved means "Merge it now"

[Zach Hoffman <zr...@apache.org>]

A number of PRs submitted in the last 6 months were approved without that
PR being merged. This has caused confusion among committers, and in some
cases, the PR has been merged before the PR approver intended the PR to be
merged. To avoid confusion in the future, it would help if we could agree
on some guidelines for approving PRs. Here is a draft:

If you approve a PR, you are vouching that:
1. You have already tested the PR
2. You will not request for the PR to be changed any further before it is
merged.
3. The PR is ready to merge *right away*, and if someone merges it, you are
okay with that and accept accountability for your approving PR review.

When not to approve the PR:
• If you have not tested a PR (especially if you intend to test it before
merging), do not approve the PR.
• If you want, or might want, the PR to be changed in some way before it is
merged, do not approve the PR.

Cases where you might approve the PR without want it to be immediately
merged:
• If you are reviewing only part of a PR and are relying on other
contributors to review the rest of the PR, approving the PR is okay as long
as you make it clear that you are only approving that specific part. For
example, if a PR affects both Traffic Ops and Traffic Portal and you only
intend to review the Traffic Portal portion, approving the PR is okay as
long as you mention that you are only approving the Traffic Portal portion
of the PR.
• If CI (GitHub Actions for us, currently) is running or pending and you
100% sure that the CI will pass, approving the PR is okay. For example, if
you have already seen that the GitHub Actions pass for that PR, then the
submitter adds an additional commit that should not keep the GitHub Actions
from passing again, approving the PR is okay.

I'm a bit reluctant to submit a PR to add it to our CONTRIBUTING.md (
https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md )
because the file is already pretty long, and at the bottom it says "Don't
let all these guidelines discourage you, we're more interested in community
involvement than perfection." IMO we should only add things that add
clarity to our expectations, rather than complicating them. This isn't a
long-standing issue, and I think the vast majority of our contributors
already see "approve" as meaning "merge it now".

Thoughts?

-Zach

Re: Approved means "Merge it now"

[John Rushford <jj...@gmail.com>]

I agree with Rob, I don't think any more rules are necessary.

John



On Mon, Jun 21, 2021 at 8:34 AM Robert O Butts <ro...@apache.org> wrote:

> I'm not really a fan of putting more rules in place around PRs. We already
> have a lot of rules around when you're allowed to make a PR, exactly what
> it has to look like, exactly what you have to put in the issue text.
>
> More specifically, it's pretty common to say "I'm approving this one part
> of this PR." People can read, I don't think we've ever had a big problem
> with PRs getting accidentally merged, because someone "Approved X but not
> Y," and people didn't read it and hit merge.
>
> I also occasionally Approve to mean "I am technically ok with this being
> merged in and maybe a Github Issue being created for some missing thing,
> but I would really like to see that thing added if the author doesn't
> mind." In which case, people should read what I wrote, and not merge it,
> unless it's clear the author isn't going to. In which case the merger also
> needs to make that Issue, not just Merge without thinking about it.
>
> In a nutshell, people Merging always need to read the issue, not just look
> for "is it approved?" And I think they do, I don't think we have a problem
> with that.
>
> The more rules we have, the harder it is to build community, the harder it
> is to get new contributors, the harder it is to keep the contributors we do
> have. If we already had a big, thriving community, and had issues with PRs
> being unmanageable, it might make more sense. But right now, we struggle
> with community building drastically more than with PR management. We need
> to be making contributing easier, not harder.
>
> I understand not having strict rules leads to occasional confusion, and as
> you say, accidental merges, and then that has to be managed. But it's
> really not that much work to revert, or make a new PR, or whatever the fix
> is. It's just git, and the `master` branch isn't release-ready anyway, it's
> not a big deal for it to not be perfect at all times.
>
> With where our project is in terms of community health, I think those minor
> inconveniences are drastically outweighed by the cost to community building
> of adding more rules around contributing.
>
>
> On Fri, Jun 18, 2021 at 5:02 PM Zach Hoffman <zr...@apache.org> wrote:
>
> > A number of PRs submitted in the last 6 months were approved without that
> > PR being merged. This has caused confusion among committers, and in some
> > cases, the PR has been merged before the PR approver intended the PR to
> be
> > merged. To avoid confusion in the future, it would help if we could agree
> > on some guidelines for approving PRs. Here is a draft:
> >
> > If you approve a PR, you are vouching that:
> > 1. You have already tested the PR
> > 2. You will not request for the PR to be changed any further before it is
> > merged.
> > 3. The PR is ready to merge *right away*, and if someone merges it, you
> are
> > okay with that and accept accountability for your approving PR review.
> >
> > When not to approve the PR:
> > • If you have not tested a PR (especially if you intend to test it before
> > merging), do not approve the PR.
> > • If you want, or might want, the PR to be changed in some way before it
> is
> > merged, do not approve the PR.
> >
> > Cases where you might approve the PR without want it to be immediately
> > merged:
> > • If you are reviewing only part of a PR and are relying on other
> > contributors to review the rest of the PR, approving the PR is okay as
> long
> > as you make it clear that you are only approving that specific part. For
> > example, if a PR affects both Traffic Ops and Traffic Portal and you only
> > intend to review the Traffic Portal portion, approving the PR is okay as
> > long as you mention that you are only approving the Traffic Portal
> portion
> > of the PR.
> > • If CI (GitHub Actions for us, currently) is running or pending and you
> > 100% sure that the CI will pass, approving the PR is okay. For example,
> if
> > you have already seen that the GitHub Actions pass for that PR, then the
> > submitter adds an additional commit that should not keep the GitHub
> Actions
> > from passing again, approving the PR is okay.
> >
> > I'm a bit reluctant to submit a PR to add it to our CONTRIBUTING.md (
> > https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md )
> > because the file is already pretty long, and at the bottom it says "Don't
> > let all these guidelines discourage you, we're more interested in
> community
> > involvement than perfection." IMO we should only add things that add
> > clarity to our expectations, rather than complicating them. This isn't a
> > long-standing issue, and I think the vast majority of our contributors
> > already see "approve" as meaning "merge it now".
> >
> > Thoughts?
> >
> > -Zach
> >
>


-- 
John Rushford
jjrushford@gmail.com

Re: Approved means "Merge it now"

[Dave Neuman <ne...@apache.org>]

I just want to re-enforce that there are no guarantees that the master
branch is production quality code.  We do our best to ensure it is, but it
is not a release branch which is production-ready code.
I say that to say that if a PR is approved and merged before it is ready,
we can just make a new PR to finish up whatever was missing. It's not that
big of a deal really.
Less process is better.

On Mon, Jun 21, 2021 at 9:09 AM Rawlin Peters <ra...@apache.org> wrote:

> I also think we shouldn't be putting more rules in place, but I'm glad
> you brought this up, Zach, because we definitely want to merge more
> PRs (which is a good thing -- we already have too many open PRs). I
> think this is a good reminder for committers to know that they're
> entrusted with the overall success of the project and that we can all
> do better to communicate our intentions with respect to PR approvals.
>
> - Rawlin
>
> On Mon, Jun 21, 2021 at 8:55 AM Jeremy Mitchell <mi...@gmail.com>
> wrote:
> >
> > I will admit, I am often tempted to merge "approved" PRs probably due to
> my
> > desire to bring down the open PR count (currently 84). And originally, I
> > agreed more with Zach and thought some sort of process/agreement was
> needed
> > around what "approved" means but after thinking about it more and reading
> > Rob's response maybe the onus is on the merger to make sure it's "ready
> to
> > merge" (by talking to the contributor and the approver(s)) OR maybe if
> i'm
> > not involved with a PR (not the contributor or an approver) I should not
> > concern myself with the PR and let contributor/approvers work to get it
> > merged.
> >
> > TLDR; if you are about to hit the merge button, make sure you talk to the
> > contributor/approvers first (or read the comments like Rob said) to make
> > sure it's ready for merge.
> >
> > On Mon, Jun 21, 2021 at 8:33 AM Robert O Butts <ro...@apache.org> wrote:
> >
> > > I'm not really a fan of putting more rules in place around PRs. We
> already
> > > have a lot of rules around when you're allowed to make a PR, exactly
> what
> > > it has to look like, exactly what you have to put in the issue text.
> > >
> > > More specifically, it's pretty common to say "I'm approving this one
> part
> > > of this PR." People can read, I don't think we've ever had a big
> problem
> > > with PRs getting accidentally merged, because someone "Approved X but
> not
> > > Y," and people didn't read it and hit merge.
> > >
> > > I also occasionally Approve to mean "I am technically ok with this
> being
> > > merged in and maybe a Github Issue being created for some missing
> thing,
> > > but I would really like to see that thing added if the author doesn't
> > > mind." In which case, people should read what I wrote, and not merge
> it,
> > > unless it's clear the author isn't going to. In which case the merger
> also
> > > needs to make that Issue, not just Merge without thinking about it.
> > >
> > > In a nutshell, people Merging always need to read the issue, not just
> look
> > > for "is it approved?" And I think they do, I don't think we have a
> problem
> > > with that.
> > >
> > > The more rules we have, the harder it is to build community, the
> harder it
> > > is to get new contributors, the harder it is to keep the contributors
> we do
> > > have. If we already had a big, thriving community, and had issues with
> PRs
> > > being unmanageable, it might make more sense. But right now, we
> struggle
> > > with community building drastically more than with PR management. We
> need
> > > to be making contributing easier, not harder.
> > >
> > > I understand not having strict rules leads to occasional confusion,
> and as
> > > you say, accidental merges, and then that has to be managed. But it's
> > > really not that much work to revert, or make a new PR, or whatever the
> fix
> > > is. It's just git, and the `master` branch isn't release-ready anyway,
> it's
> > > not a big deal for it to not be perfect at all times.
> > >
> > > With where our project is in terms of community health, I think those
> minor
> > > inconveniences are drastically outweighed by the cost to community
> building
> > > of adding more rules around contributing.
> > >
> > >
> > > On Fri, Jun 18, 2021 at 5:02 PM Zach Hoffman <zr...@apache.org>
> wrote:
> > >
> > > > A number of PRs submitted in the last 6 months were approved without
> that
> > > > PR being merged. This has caused confusion among committers, and in
> some
> > > > cases, the PR has been merged before the PR approver intended the PR
> to
> > > be
> > > > merged. To avoid confusion in the future, it would help if we could
> agree
> > > > on some guidelines for approving PRs. Here is a draft:
> > > >
> > > > If you approve a PR, you are vouching that:
> > > > 1. You have already tested the PR
> > > > 2. You will not request for the PR to be changed any further before
> it is
> > > > merged.
> > > > 3. The PR is ready to merge *right away*, and if someone merges it,
> you
> > > are
> > > > okay with that and accept accountability for your approving PR
> review.
> > > >
> > > > When not to approve the PR:
> > > > • If you have not tested a PR (especially if you intend to test it
> before
> > > > merging), do not approve the PR.
> > > > • If you want, or might want, the PR to be changed in some way
> before it
> > > is
> > > > merged, do not approve the PR.
> > > >
> > > > Cases where you might approve the PR without want it to be
> immediately
> > > > merged:
> > > > • If you are reviewing only part of a PR and are relying on other
> > > > contributors to review the rest of the PR, approving the PR is okay
> as
> > > long
> > > > as you make it clear that you are only approving that specific part.
> For
> > > > example, if a PR affects both Traffic Ops and Traffic Portal and you
> only
> > > > intend to review the Traffic Portal portion, approving the PR is
> okay as
> > > > long as you mention that you are only approving the Traffic Portal
> > > portion
> > > > of the PR.
> > > > • If CI (GitHub Actions for us, currently) is running or pending and
> you
> > > > 100% sure that the CI will pass, approving the PR is okay. For
> example,
> > > if
> > > > you have already seen that the GitHub Actions pass for that PR, then
> the
> > > > submitter adds an additional commit that should not keep the GitHub
> > > Actions
> > > > from passing again, approving the PR is okay.
> > > >
> > > > I'm a bit reluctant to submit a PR to add it to our CONTRIBUTING.md (
> > > > https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md
> )
> > > > because the file is already pretty long, and at the bottom it says
> "Don't
> > > > let all these guidelines discourage you, we're more interested in
> > > community
> > > > involvement than perfection." IMO we should only add things that add
> > > > clarity to our expectations, rather than complicating them. This
> isn't a
> > > > long-standing issue, and I think the vast majority of our
> contributors
> > > > already see "approve" as meaning "merge it now".
> > > >
> > > > Thoughts?
> > > >
> > > > -Zach
> > > >
> > >
>

Re: Approved means "Merge it now"

[Rawlin Peters <ra...@apache.org>]

I also think we shouldn't be putting more rules in place, but I'm glad
you brought this up, Zach, because we definitely want to merge more
PRs (which is a good thing -- we already have too many open PRs). I
think this is a good reminder for committers to know that they're
entrusted with the overall success of the project and that we can all
do better to communicate our intentions with respect to PR approvals.

- Rawlin

On Mon, Jun 21, 2021 at 8:55 AM Jeremy Mitchell <mi...@gmail.com> wrote:
>
> I will admit, I am often tempted to merge "approved" PRs probably due to my
> desire to bring down the open PR count (currently 84). And originally, I
> agreed more with Zach and thought some sort of process/agreement was needed
> around what "approved" means but after thinking about it more and reading
> Rob's response maybe the onus is on the merger to make sure it's "ready to
> merge" (by talking to the contributor and the approver(s)) OR maybe if i'm
> not involved with a PR (not the contributor or an approver) I should not
> concern myself with the PR and let contributor/approvers work to get it
> merged.
>
> TLDR; if you are about to hit the merge button, make sure you talk to the
> contributor/approvers first (or read the comments like Rob said) to make
> sure it's ready for merge.
>
> On Mon, Jun 21, 2021 at 8:33 AM Robert O Butts <ro...@apache.org> wrote:
>
> > I'm not really a fan of putting more rules in place around PRs. We already
> > have a lot of rules around when you're allowed to make a PR, exactly what
> > it has to look like, exactly what you have to put in the issue text.
> >
> > More specifically, it's pretty common to say "I'm approving this one part
> > of this PR." People can read, I don't think we've ever had a big problem
> > with PRs getting accidentally merged, because someone "Approved X but not
> > Y," and people didn't read it and hit merge.
> >
> > I also occasionally Approve to mean "I am technically ok with this being
> > merged in and maybe a Github Issue being created for some missing thing,
> > but I would really like to see that thing added if the author doesn't
> > mind." In which case, people should read what I wrote, and not merge it,
> > unless it's clear the author isn't going to. In which case the merger also
> > needs to make that Issue, not just Merge without thinking about it.
> >
> > In a nutshell, people Merging always need to read the issue, not just look
> > for "is it approved?" And I think they do, I don't think we have a problem
> > with that.
> >
> > The more rules we have, the harder it is to build community, the harder it
> > is to get new contributors, the harder it is to keep the contributors we do
> > have. If we already had a big, thriving community, and had issues with PRs
> > being unmanageable, it might make more sense. But right now, we struggle
> > with community building drastically more than with PR management. We need
> > to be making contributing easier, not harder.
> >
> > I understand not having strict rules leads to occasional confusion, and as
> > you say, accidental merges, and then that has to be managed. But it's
> > really not that much work to revert, or make a new PR, or whatever the fix
> > is. It's just git, and the `master` branch isn't release-ready anyway, it's
> > not a big deal for it to not be perfect at all times.
> >
> > With where our project is in terms of community health, I think those minor
> > inconveniences are drastically outweighed by the cost to community building
> > of adding more rules around contributing.
> >
> >
> > On Fri, Jun 18, 2021 at 5:02 PM Zach Hoffman <zr...@apache.org> wrote:
> >
> > > A number of PRs submitted in the last 6 months were approved without that
> > > PR being merged. This has caused confusion among committers, and in some
> > > cases, the PR has been merged before the PR approver intended the PR to
> > be
> > > merged. To avoid confusion in the future, it would help if we could agree
> > > on some guidelines for approving PRs. Here is a draft:
> > >
> > > If you approve a PR, you are vouching that:
> > > 1. You have already tested the PR
> > > 2. You will not request for the PR to be changed any further before it is
> > > merged.
> > > 3. The PR is ready to merge *right away*, and if someone merges it, you
> > are
> > > okay with that and accept accountability for your approving PR review.
> > >
> > > When not to approve the PR:
> > > • If you have not tested a PR (especially if you intend to test it before
> > > merging), do not approve the PR.
> > > • If you want, or might want, the PR to be changed in some way before it
> > is
> > > merged, do not approve the PR.
> > >
> > > Cases where you might approve the PR without want it to be immediately
> > > merged:
> > > • If you are reviewing only part of a PR and are relying on other
> > > contributors to review the rest of the PR, approving the PR is okay as
> > long
> > > as you make it clear that you are only approving that specific part. For
> > > example, if a PR affects both Traffic Ops and Traffic Portal and you only
> > > intend to review the Traffic Portal portion, approving the PR is okay as
> > > long as you mention that you are only approving the Traffic Portal
> > portion
> > > of the PR.
> > > • If CI (GitHub Actions for us, currently) is running or pending and you
> > > 100% sure that the CI will pass, approving the PR is okay. For example,
> > if
> > > you have already seen that the GitHub Actions pass for that PR, then the
> > > submitter adds an additional commit that should not keep the GitHub
> > Actions
> > > from passing again, approving the PR is okay.
> > >
> > > I'm a bit reluctant to submit a PR to add it to our CONTRIBUTING.md (
> > > https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md )
> > > because the file is already pretty long, and at the bottom it says "Don't
> > > let all these guidelines discourage you, we're more interested in
> > community
> > > involvement than perfection." IMO we should only add things that add
> > > clarity to our expectations, rather than complicating them. This isn't a
> > > long-standing issue, and I think the vast majority of our contributors
> > > already see "approve" as meaning "merge it now".
> > >
> > > Thoughts?
> > >
> > > -Zach
> > >
> >

Re: Approved means "Merge it now"

[Jeremy Mitchell <mi...@gmail.com>]

I will admit, I am often tempted to merge "approved" PRs probably due to my
desire to bring down the open PR count (currently 84). And originally, I
agreed more with Zach and thought some sort of process/agreement was needed
around what "approved" means but after thinking about it more and reading
Rob's response maybe the onus is on the merger to make sure it's "ready to
merge" (by talking to the contributor and the approver(s)) OR maybe if i'm
not involved with a PR (not the contributor or an approver) I should not
concern myself with the PR and let contributor/approvers work to get it
merged.

TLDR; if you are about to hit the merge button, make sure you talk to the
contributor/approvers first (or read the comments like Rob said) to make
sure it's ready for merge.

On Mon, Jun 21, 2021 at 8:33 AM Robert O Butts <ro...@apache.org> wrote:

> I'm not really a fan of putting more rules in place around PRs. We already
> have a lot of rules around when you're allowed to make a PR, exactly what
> it has to look like, exactly what you have to put in the issue text.
>
> More specifically, it's pretty common to say "I'm approving this one part
> of this PR." People can read, I don't think we've ever had a big problem
> with PRs getting accidentally merged, because someone "Approved X but not
> Y," and people didn't read it and hit merge.
>
> I also occasionally Approve to mean "I am technically ok with this being
> merged in and maybe a Github Issue being created for some missing thing,
> but I would really like to see that thing added if the author doesn't
> mind." In which case, people should read what I wrote, and not merge it,
> unless it's clear the author isn't going to. In which case the merger also
> needs to make that Issue, not just Merge without thinking about it.
>
> In a nutshell, people Merging always need to read the issue, not just look
> for "is it approved?" And I think they do, I don't think we have a problem
> with that.
>
> The more rules we have, the harder it is to build community, the harder it
> is to get new contributors, the harder it is to keep the contributors we do
> have. If we already had a big, thriving community, and had issues with PRs
> being unmanageable, it might make more sense. But right now, we struggle
> with community building drastically more than with PR management. We need
> to be making contributing easier, not harder.
>
> I understand not having strict rules leads to occasional confusion, and as
> you say, accidental merges, and then that has to be managed. But it's
> really not that much work to revert, or make a new PR, or whatever the fix
> is. It's just git, and the `master` branch isn't release-ready anyway, it's
> not a big deal for it to not be perfect at all times.
>
> With where our project is in terms of community health, I think those minor
> inconveniences are drastically outweighed by the cost to community building
> of adding more rules around contributing.
>
>
> On Fri, Jun 18, 2021 at 5:02 PM Zach Hoffman <zr...@apache.org> wrote:
>
> > A number of PRs submitted in the last 6 months were approved without that
> > PR being merged. This has caused confusion among committers, and in some
> > cases, the PR has been merged before the PR approver intended the PR to
> be
> > merged. To avoid confusion in the future, it would help if we could agree
> > on some guidelines for approving PRs. Here is a draft:
> >
> > If you approve a PR, you are vouching that:
> > 1. You have already tested the PR
> > 2. You will not request for the PR to be changed any further before it is
> > merged.
> > 3. The PR is ready to merge *right away*, and if someone merges it, you
> are
> > okay with that and accept accountability for your approving PR review.
> >
> > When not to approve the PR:
> > • If you have not tested a PR (especially if you intend to test it before
> > merging), do not approve the PR.
> > • If you want, or might want, the PR to be changed in some way before it
> is
> > merged, do not approve the PR.
> >
> > Cases where you might approve the PR without want it to be immediately
> > merged:
> > • If you are reviewing only part of a PR and are relying on other
> > contributors to review the rest of the PR, approving the PR is okay as
> long
> > as you make it clear that you are only approving that specific part. For
> > example, if a PR affects both Traffic Ops and Traffic Portal and you only
> > intend to review the Traffic Portal portion, approving the PR is okay as
> > long as you mention that you are only approving the Traffic Portal
> portion
> > of the PR.
> > • If CI (GitHub Actions for us, currently) is running or pending and you
> > 100% sure that the CI will pass, approving the PR is okay. For example,
> if
> > you have already seen that the GitHub Actions pass for that PR, then the
> > submitter adds an additional commit that should not keep the GitHub
> Actions
> > from passing again, approving the PR is okay.
> >
> > I'm a bit reluctant to submit a PR to add it to our CONTRIBUTING.md (
> > https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md )
> > because the file is already pretty long, and at the bottom it says "Don't
> > let all these guidelines discourage you, we're more interested in
> community
> > involvement than perfection." IMO we should only add things that add
> > clarity to our expectations, rather than complicating them. This isn't a
> > long-standing issue, and I think the vast majority of our contributors
> > already see "approve" as meaning "merge it now".
> >
> > Thoughts?
> >
> > -Zach
> >
>

Re: Approved means "Merge it now"

[Robert O Butts <ro...@apache.org>]

I'm not really a fan of putting more rules in place around PRs. We already
have a lot of rules around when you're allowed to make a PR, exactly what
it has to look like, exactly what you have to put in the issue text.

More specifically, it's pretty common to say "I'm approving this one part
of this PR." People can read, I don't think we've ever had a big problem
with PRs getting accidentally merged, because someone "Approved X but not
Y," and people didn't read it and hit merge.

I also occasionally Approve to mean "I am technically ok with this being
merged in and maybe a Github Issue being created for some missing thing,
but I would really like to see that thing added if the author doesn't
mind." In which case, people should read what I wrote, and not merge it,
unless it's clear the author isn't going to. In which case the merger also
needs to make that Issue, not just Merge without thinking about it.

In a nutshell, people Merging always need to read the issue, not just look
for "is it approved?" And I think they do, I don't think we have a problem
with that.

The more rules we have, the harder it is to build community, the harder it
is to get new contributors, the harder it is to keep the contributors we do
have. If we already had a big, thriving community, and had issues with PRs
being unmanageable, it might make more sense. But right now, we struggle
with community building drastically more than with PR management. We need
to be making contributing easier, not harder.

I understand not having strict rules leads to occasional confusion, and as
you say, accidental merges, and then that has to be managed. But it's
really not that much work to revert, or make a new PR, or whatever the fix
is. It's just git, and the `master` branch isn't release-ready anyway, it's
not a big deal for it to not be perfect at all times.

With where our project is in terms of community health, I think those minor
inconveniences are drastically outweighed by the cost to community building
of adding more rules around contributing.


On Fri, Jun 18, 2021 at 5:02 PM Zach Hoffman <zr...@apache.org> wrote:

> A number of PRs submitted in the last 6 months were approved without that
> PR being merged. This has caused confusion among committers, and in some
> cases, the PR has been merged before the PR approver intended the PR to be
> merged. To avoid confusion in the future, it would help if we could agree
> on some guidelines for approving PRs. Here is a draft:
>
> If you approve a PR, you are vouching that:
> 1. You have already tested the PR
> 2. You will not request for the PR to be changed any further before it is
> merged.
> 3. The PR is ready to merge *right away*, and if someone merges it, you are
> okay with that and accept accountability for your approving PR review.
>
> When not to approve the PR:
> • If you have not tested a PR (especially if you intend to test it before
> merging), do not approve the PR.
> • If you want, or might want, the PR to be changed in some way before it is
> merged, do not approve the PR.
>
> Cases where you might approve the PR without want it to be immediately
> merged:
> • If you are reviewing only part of a PR and are relying on other
> contributors to review the rest of the PR, approving the PR is okay as long
> as you make it clear that you are only approving that specific part. For
> example, if a PR affects both Traffic Ops and Traffic Portal and you only
> intend to review the Traffic Portal portion, approving the PR is okay as
> long as you mention that you are only approving the Traffic Portal portion
> of the PR.
> • If CI (GitHub Actions for us, currently) is running or pending and you
> 100% sure that the CI will pass, approving the PR is okay. For example, if
> you have already seen that the GitHub Actions pass for that PR, then the
> submitter adds an additional commit that should not keep the GitHub Actions
> from passing again, approving the PR is okay.
>
> I'm a bit reluctant to submit a PR to add it to our CONTRIBUTING.md (
> https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md )
> because the file is already pretty long, and at the bottom it says "Don't
> let all these guidelines discourage you, we're more interested in community
> involvement than perfection." IMO we should only add things that add
> clarity to our expectations, rather than complicating them. This isn't a
> long-standing issue, and I think the vast majority of our contributors
> already see "approve" as meaning "merge it now".
>
> Thoughts?
>
> -Zach
>