governance: updating review changes#30995
Conversation
CONTRIBUTING.md
Outdated
There was a problem hiding this comment.
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.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
CONTRIBUTING.md
Outdated
There was a problem hiding this comment.
inlined this list above
CONTRIBUTING.md
Outdated
| organization specific shortcuts into the code. | ||
| organization specific shortcuts into the code. This is not required for less | ||
| contentious PRs like test-only changes, simple refactors, and bug fixes. | ||
| HTTP/3 changes are largely exempt from cross-company approvals as all of the |
There was a problem hiding this comment.
HTTP/3 we discussed a long time back on maintainers but I'd like it codified so folks don't think we're cutting corners on revew.
|
@envoyproxy/envoy-maintainers as mentioned on slack, PTAL and please share any concerns, suggestions, etc. |
| * 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 |
There was a problem hiding this comment.
extensions is a bit vague here, since we split many core feature as extensions, e.g. HCM, TLS. Let's say contrib extensions?
There was a problem hiding this comment.
+1, does it mean all extensions or non core extensions?
There was a problem hiding this comment.
Can we clarify what is "core code" / "core extensions" refers to especially given the points made above
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
HCM code is already in code but I can move the config files too, good call.
There was a problem hiding this comment.
filed #31113 folks can add things there either now, or later on as we think something needs more careful review.
There was a problem hiding this comment.
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.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
CONTRIBUTING.md
Outdated
| should be from an organization other than Lyft. This helps us make sure that we aren't putting | ||
| organization specific shortcuts into the code. | ||
| organization specific shortcuts into the code. This is not required for less | ||
| contentious PRs like test-only changes, simple refactors, and bug fixes. |
There was a problem hiding this comment.
Is this comment about "less contentious PRs" redundant with the language on line 240?
CONTRIBUTING.md
Outdated
| organization specific shortcuts into the code. | ||
| organization specific shortcuts into the code. This is not required for less | ||
| contentious PRs like test-only changes, simple refactors, and bug fixes. | ||
| HTTP/3 changes are largely exempt from cross-company approvals as all of the |
There was a problem hiding this comment.
nit: I think this should probably be a new bullet point, since HTTP/3 is not a new feature?
|
we OK with this as-is given the plan to move code? |
Risk Level: n/a Testing: n/a Docs Changes: yes Release Notes: no Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Risk Level: n/a
Testing: n/a
Docs Changes: yes
Release Notes: no