Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
docs: API review checklist #14399
docs: API review checklist #14399
Changes from all commits
5cd1b54
f85cba7
6c90c34
8821b77
c4ceee7
1523c98
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think much of this overlaps in spirit with the GOVERNANCE runtime guard section. I know the API stuff is different since it affects more than just envoy, but it'd still be good to address the overlap, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I think we should port the relevant parts of GOVERNANCE.md across to an API GOVERNANCE.md; it might be copy+paste.
We are working towards a multi-stakeholder governance for the APIs (under CNCF, see https://github.com/cncf/udpa/blob/master/README.md), so we should evolve the API docs with that in mind.
This might seem a minor thing, but I think this decoupling is critical to ensuring that other adopters of xDS are comfortable and don't feel locked into Envoy project governance. We will do this while retaining the agree upon foundations from GOVERNANCE.md.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a governance file in cncf/xds and just point at that from api/GOVERNANCE.md in this repo, to make it clear that it's a separate thing? Actually, for that matter, should we do the same thing with files like this one and api/STYLE.md as well?
In any case, I agree that we should document the governance of the API and get all of these docs properly aligned, but I think a lot of this is outside the scope of this PR. Can we follow up on that separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SG, @markdroth can you file an issue on cncf/xds around this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cncf/xds#1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can enforce any of these with fix_format / proto scripts. If we had a required required/not_required annotation, or required "unbounded" annotations, so folks had to explicitly annotate, it'd be annoying but might catch more.
I'd file this under "food for thought" where you and the other shephards can decide if it's worth it after seeing what slips through review over time :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let @htuch think about this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be done. I think if I had my wishlist of tooling, we would have something that during code review annotates the PR when a new field is introduced and is missing constraints to verify.
@markdroth can you be more explicit and say something like "Is the field mandatory? If so, are protocgen-validate constraints defined to enforce this?".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a separate bullet about having PGV rules, since that basically applies to all of the bullets in this section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't we traditionally added a new field and deprecated old ones in the same PR? I guess better project party is part of pulling the APIs out here. I think it'd be helpful if before we codify this rule, we have some process around adding a field, making sure the migrations are done in both projects, being able to determine when they're done and having issues filed to do the deprecation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can play this by ear. Right now, there are only 2 clients, and the API shepherds all talk to each other, so it's fairly easy to coordinate this without much red tape. Over time, if this becomes more of a problem, we can add more process as appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we redact this for now or add an "or get sign off from each project stakeholder for immediate deprecation"? Given n=2 I think that's less onerous and as we add more stakeholders I think we'd have to move towards the deprecation dance anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a sentence indicating to ask the API shepherds if you're not sure about the status of a feature in the major known clients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'm on board with playing this by ear, but the documentation this still disallows changes like I mentioned. I want us to be explicit here. I don't want to pedantic, but I really want to avoid having codified rules we encourage people in the know to ignore. If we're Ok playing it by ear, let's remove the restriction that you can't deprecate a field until the replacement is ready in all consuming stacks. If we're not OK doing this with shephard approval, we should make it clear it is now disallowed to add A and deprecate B in the same PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, okay, sorry -- I somehow didn't grok that it was the ability to explicitly sign off on exceptions that you wanted to call out. I've added a clause about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mention
TypedExtensionConfig
somewhere?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that I'd like to do a better job of is placing protos that might eventually become shared in shared locations to begin with. We've seen historically situations where we have had to copy+paste protos to common directories or refer to protos in strange locations (e.g. some rando filter referring to the access log matcher in the access log tree). This advice also includes thinking carefully about whether some sub-messages might be one day useful elsewhere.
It's hard to be super prescriptive, but good to be thoughtful about. Basically, without major versioning as a device, refactors are messy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a bullet about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another bullet point "If this change involves extension configuration, how will it interact with ECDS?" CC @kyessenov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think config size scalability is also a huge deal. We should make sure we're careful not to add things that might blow out config size, e.g. some huge proto that is mandatory and attached to every single route entry, or things that are expensive during config ingestion, such as JSON round-trips.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a couple of bullets about this.