Skip to content

governance: updating review changes #30995

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
alyssawilk merged 4 commits into from
Dec 5, 2023
Merged

governance: updating review changes #30995

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6f30fdc
governance: updating review changes
alyssawilk Nov 21, 2023
f11d41e
more detail for contrib
alyssawilk Nov 29, 2023
3695238
further
alyssawilk Nov 29, 2023
883ab7f
comments
alyssawilk Nov 29, 2023
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
15 changes: 8 additions & 7 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,18 +237,19 @@ Please note that if adding a runtime guarded feature, your [release notes](chang
* Typically we try to turn around reviews within one business day.
* See [OWNERS.md](OWNERS.md) for the current list of maintainers.
* It is generally expected that a senior maintainer should review every PR to
core code. Test-only or extension-only changes need only be reviewed by a
maintainer, or senior extension maintainer.
core code. Changes which only touch tests, extensions, tools, docs or comments
Copy link
Copy Markdown
Member

@lizan lizan Nov 22, 2023

Choose a reason for hiding this comment

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

extensions is a bit vague here, since we split many core feature as extensions, e.g. HCM, TLS. Let's say contrib extensions?

Copy link
Copy Markdown
Member

@nezdolik nezdolik Nov 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

+1, does it mean all extensions or non core extensions?

Copy link
Copy Markdown
Contributor

@KBaichoo KBaichoo Nov 22, 2023

Choose a reason for hiding this comment

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

Can we clarify what is "core code" / "core extensions" refers to especially given the points made above

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Nov 28, 2023

Choose a reason for hiding this comment

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

hm, what if we move the "core" code back out of extensions? E-M doesn't care where it lives as long as it isn't linked in. then we have a very clear "if it's in this directory it doesn't need senior review" guidance.

Copy link
Copy Markdown
Member

@ggreenway ggreenway Nov 28, 2023

Choose a reason for hiding this comment

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

I agree that some extensions are more critical than others.

I also agree that indicating that via directory structure makes a lot of sense, so it's easy to tell the difference without needing to consult a list or something.

But my understanding is that some of the previously-core code was moved to extensions so we could use the existing build system mechanism to exclude it easily (intended for E-M, but may be useful in other cases).

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Nov 29, 2023

Choose a reason for hiding this comment

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

Probably all of the TCP/HTTP connection pool/upstream stuff? I haven't kept up to date with everything that was split out but I would say in general anything that is required for the basic HTTP proxy case, including HCM.

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Nov 29, 2023

Choose a reason for hiding this comment

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

HCM code is already in code but I can move the config files too, good call.

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Nov 29, 2023

Choose a reason for hiding this comment

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

filed #31113 folks can add things there either now, or later on as we think something needs more careful review.

Copy link
Copy Markdown
Member

@ggreenway ggreenway Nov 29, 2023

Choose a reason for hiding this comment

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

I think I'd include TLS as well; it's always been an extension, but it's extremely widely used, and easy to mess up in ways that cause security bugs.

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Nov 29, 2023

Choose a reason for hiding this comment

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

SGTM

need only be reviewed by a maintainer, or senior extension maintainer.
* It is also generally expected that a "domain expert" for the code the PR touches should review the
PR. This person does not necessarily need to have commit access.
* The previous two points generally mean that every PR should have two approvals. (Exceptions can
Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Nov 21, 2023

Choose a reason for hiding this comment

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

I think we regularly end up with a cross-company domain review by a senior maintainer so I'm not sure this one really applies.

be made by the senior maintainers).
* The above rules may be waived for PRs which only update docs or comments, or trivial changes to
Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Nov 21, 2023

Choose a reason for hiding this comment

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

inlined this list above

tests and tools (where trivial is decided by the maintainer in question).
* In general, we should also attempt to make sure that at least one of the approvals is *from an
* For new extensions (contrib or otherwise) and features, at least one of the approvals should be *from an
organization different from the PR author.* E.g., if Lyft authors a PR, at least one approver
should be from an organization other than Lyft. This helps us make sure that we aren't putting
organization specific shortcuts into the code.
new HTTP/3 features are largely exempt from cross-company approvals as all of the
area experts work at a single company, but HTTP/3 changes which impact general
functionality still merit a cross-company check.
* contrib extensions do not need senior maintainer or maintainer review only contrib owner review and
a maintainer stamp to merge.
* If there is a question on who should review a PR please discuss in Slack.
* Anyone is welcome to review any PR that they want, whether they are a maintainer or not.
* Please make sure that the PR title, commit message, and description are updated if the PR changes
Expand Down