First draft of a document outlining the api review process #419
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
pwittrock
force-pushed
the
design
branch
3 times, most recently
from
cc46177
to
4467884
Compare
|
@pwittrock Thanks for putting this together. I'm starting to review it in detail, but first a quick comment: We'll need to describe how this relates to:
|
|
The language in api_changes.md implies that an API will typically transition from beta to stable/GA in the immediately subsequent release. Given our release schedule, that allows for zero user feedback. I'd like to require at least one full release cycle at beta before advancing to GA. |
|
|
||
| **Include any deadlines that are related.** e.g. another feature depends on this and it is slated for 1.x. | ||
|
|
||
| ## Proposed changes for new types |
There was a problem hiding this comment.
I think we should combine the new and existing type cases and just call out differences between the two, in order to reduce duplication.
| - As a cluster administrator... | ||
| - As a application developer... | ||
| - As a Kubernetes extension developer... | ||
|
|
There was a problem hiding this comment.
An entirely new API should have a requirements section (at least functional ones, ideally non-functional also).
| For any list fields, describe whether they will be replaced when patched or merged. If merged, | ||
| describe the merge key that will be used. | ||
|
|
||
| ## Impact on backwards / forwards compatibility |
There was a problem hiding this comment.
Maybe we should expand this compatibility section into a checklist of gotchas:
- API conventions deviations
- Backwards/forwards compatibility
- Known future incompatibility risks
- Apply compatibility
- Expected imperative orchestration scenarios
- Security and PII
- Other client library, CLI, and UI considerations
|
|
||
| ## Alternatives considered | ||
|
|
||
| List alternative solutions to the use cases. |
There was a problem hiding this comment.
Especially client-side solutions and TPR.
And similarities to APIs in other similar systems.
| @@ -0,0 +1,84 @@ | |||
| # Description of your Api | |||
|
|
|||
There was a problem hiding this comment.
Somewhere early we should call out the stage of the API the proposal would change (alpha, beta, GA), and what repo it affects.
|
|
||
| Design approvers: | ||
| - @smarterclayton | ||
| - @bgrant |
|
|
||
| When writing a design proposal, you must consider the following: | ||
|
|
||
| ### Api version semantics |
There was a problem hiding this comment.
Consolidate with existing api_changes.md section.
| | autoscaling | | | ||
| | batch | | | ||
| | certificates | | | ||
| | componentconfig | | |
There was a problem hiding this comment.
componentconfig is not really an API group
contributors/sig-ownership.md
Outdated
| ## Release tracking | ||
|
|
||
| The owning special interest group must create an issue in [kubernetes/features] and respond to comments | ||
| on the feature. The feature issue MUST be updated with the current status within 1-2 days after feature freeze. |
There was a problem hiding this comment.
Link to release process doc (https://github.com/kubernetes/community/blob/master/contributors/devel/release/README.md ?), which should describe feature freeze, etc.
| Any changes that break compatibility with client-server version skew tests of +-1 minor release must be resolved. | ||
|
|
||
| ## Documentation | ||
|
|
There was a problem hiding this comment.
Link to kubernetes.github.io contribution guide?
|
cc @jbeda |
|
Thanks to @jbeda to pointing out models for role clarification, which would help the API proposal process, the proposal process more broadly, and the features process: |
|
Comments partially addressed, will continue working on this and integrating / linking to the referenced docs. |
|
At a glance, it looks like OARP is fairly consistent with our existing terminology and processes. |
contributors/devel/api_changes.md
Outdated
| @@ -623,8 +623,11 @@ tips](../api.md#v1-conversion-tips)) | |||
| via a flag should not create new bugs in unrelated features; because the feature | |||
| is new, it may have minor bugs | |||
| - Support: the project commits to complete the feature, in some form, in a | |||
| subsequent Stable version; typically this will happen within 3 months, but | |||
| sometimes longer; releases should simultaneously support two consecutive | |||
| subsequent Stable version; beta features are required to remain in beta for | |||
There was a problem hiding this comment.
This is overly precise, I think. Not every feature needs to follow the same timeline?
There was a problem hiding this comment.
Updated to say this is a guideline. WDYT?
|
when components moved to versioned configuration (in what we currently call componentconfig but what i think is planned for moving), will we require the same process? same question with versioned admission controller configuration? |
|
@derekparker Good question. I don't have much state on components or controller configuration. Does this deserve a separate doc or section? What do you think we should say about this? |
|
Sorry for dropping this for over a month. Updated with the last comments. Also added a link to the ownership roles posted by @bgrant0607 / @jbeda |
|
I have no comments other than this is great. I'm sure i'll find more on subsequent readings, but don't block on my account. |
|
Agree with Clayton.
What I ultimately want to see is more overt thought on these topics, and
some recognition from some plurality of the API guardians on basically
every API change.
…On Wed, Apr 5, 2017 at 2:02 PM, Clayton Coleman ***@***.***> wrote:
I have no comments other than this is great. I'm sure i'll find more on
subsequent readings, but don't block on my account.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#419 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFVgVDH3ZIyHOlNFVah1KGyMI36ApZHwks5rtAFwgaJpZM4MQDaY>
.
|
|
Squashed commits and rebased against master HEAD. |
|
@pwittrock - I think it's not worth blocking on my question as I agree this is good stuff. My concern was in 1.7 I know there is a goal of moving kubelet configuration away from flags to a typed object. I wasn't sure if this group of reviewers wanted to also require the same level of review for what was previously a flag. It felt like that could potentially remain the domain of the owning sig if it caused too much noise. |
|
@derekparker Thanks for the context. Lets follow up on that. Does it sound like something we should discuss as with the community? |
There was a problem hiding this comment.
Overall this looks OK but I think we need to pull the API approvers list out of this doc OR acknowledge the issue of add/removals like we have done in the product security process.
I also think someone should try and do this process for something we have in-flight before ratifying it. Perhaps @Kargakis's Deployment issue?
|
|
||
| Schedule a time for your design to be reviewed. | ||
|
|
||
| Design approvers: |
There was a problem hiding this comment.
We MUST link externally to a design approvers document for community transparency. This document should essentially state: "these are the current reviewers, we are still figuring out how to add/remove people to this list, see the governance discussion".
One of the most confusing things about the API process today is that we have inconsistent lists and no central doc that explains what the team is, etc.
|
|
||
| ## How to escalate | ||
|
|
||
| Escalation should be done through the owning SIG. If you need help getting attention, reach out to your |
There was a problem hiding this comment.
can you enumerate each SIG and the APIs they own?
There was a problem hiding this comment.
Where do you think is the proper place for this to live? I could create a document, but it will be constantly stale. WDYT of comments in the code that would could lint for?
There was a problem hiding this comment.
Alternatively, if SIGs own whole api groups, it would be more manageable to add them to the table that follows.
There was a problem hiding this comment.
I don't know where it should live. @bgrant0607 made some reference to an effort to map code to owners.
|
PTAL |
| Escalation should be done through the owning SIG. If you need help getting attention, reach out to your | ||
| SIG. | ||
|
|
||
| FOr a |
|
I'd like to merge this and iterate, with the following caveats:
|
|
@calebamiles also pointed at the Rust RFC process, which I agree looks like a good starting point for the broader proposal process. https://github.com/rust-lang/rfcs#what-the-process-is |
|
@calebamiles Is this something you could help flesh out more? |
|
I agree with Brian. Disagreements? |
Address PR comments
|
SGTM. Updated to fix the cruft. I will follow up with the following documents:
|
There was a problem hiding this comment.
@bgrant0607 In the absence of having a list of API reviewers can we please document them in this document? It is still really confusing to put a process in place telling people to ping a GitHub alias and not knowing an escalation process or who is on the list.
Also, I would like to see the next iteration talk about the failure case: what happens if you far exceed your expected time for decision and still don't get a definitive no?
|
|
||
| ## How to escalate | ||
|
|
||
| Escalation should be done through the owning SIG. If you need help getting attention, reach out to your |
There was a problem hiding this comment.
I don't know where it should live. @bgrant0607 made some reference to an effort to map code to owners.
|
How about we document that we will turn API reviewers into a list in the
top level OWNERS in the near term? I don't think this doc should have the
people, just document that at the time of writing we use the list in GitHub
and it will be moved into the OWNERS list in a follow up PR.
…On Wed, Apr 19, 2017 at 6:07 PM, Brandon Philips ***@***.***> wrote:
***@***.**** approved this pull request.
@bgrant0607 <https://github.com/bgrant0607> In the absence of having a
list of API reviewers
<https://groups.google.com/forum/#!topic/kubernetes-sig-api-machinery/sq9Jwgeyy7Q>
can we please document them in this document? It is still really confusing
to put a process in place telling people to ping a GitHub alias and not
knowing an escalation process or who is on the list.
Also, I would like to see the next iteration talk about the failure case:
what happens if you far exceed your expected time for decision and still
don't get a definitive no?
------------------------------
In contributors/devel/contributing-api-changes.md
<#419 (comment)>:
> +
+Schedule a time for your design to be reviewed.
+
+Design approvers:
+- @smarterclayton
+- @bgrant0607
+- @thockin
+- @erictune
+
+### Implement the proposal
+
+Once the proposal has been accepted an merged, you can begin implementing the solution.
+
+## How to escalate
+
+Escalation should be done through the owning SIG. If you need help getting attention, reach out to your
I don't know where it should live. @bgrant0607
<https://github.com/bgrant0607> made some reference to an effort to map
code to owners.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#419 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p8MmJGcRK7YfanTMTWTW4Z00njNpks5rxoWUgaJpZM4MQDaY>
.
|
|
|
||
| *Use cases* | ||
|
|
||
| - Use cases for transitioning to Beta |
There was a problem hiding this comment.
s/Beta/GA/
Do we really need beta & GA requirements up front? If people knew what they needed to do they would just do it to begin with.
|
|
||
| Declare the proposed Group / Version / Kind for any new types created as part of this proposal. | ||
|
|
||
| Declare any new fields for existing types and their version. Will the fields be added as first class fields or |
There was a problem hiding this comment.
remove the bit about annotations, I don't want to present that as a first-class option.
|
|
||
| Is server side garbage collection needed / enabled? | ||
|
|
||
| ## Alternatives considered |
There was a problem hiding this comment.
It seems to me that some of the items in this list really belong in a proposal or design doc. I would consider focusing this doc on things that are really unique to the api, as it is feeling a little heavyweight.
|
|
||
| ### Defaulting | ||
|
|
||
| Describe defaulting that will be done |
There was a problem hiding this comment.
This should be covered by the comments in the PR adding the types, no? Same with validation below. I think we shouldn't make people describe in prose things that are best written in the documentation. (so, checking these things should be in the instructions for api reviewers.)
There was a problem hiding this comment.
Are you suggesting the defaulting and validation review be done separately in a second PR, or that the proposal PR include a link to a types.go PR, or that defaulting / validation may be reviewed by a separate group of folks?
|
Is API review going to be a function of SIG Architecture? I think it should be. |
bgrant0607
added
the
sig/architecture
k8s-github-robot
added
the
size/L
…-autoscaling 1.8 release notes for SIG Autoscaling and SIG Instrumentation
|
I'm going to resuscitate this shortly, taking from it. |
No description provided.