Adding API Review process and diagram #2433
Conversation
jdumars
added
the
sig/architecture
k8s-ci-robot
added
size/L
|
|
||
| * Any new version of a stable API | ||
|
|
||
| * Any major new functionality added to a stable API as defined by SIG Architecture and the API Reviewers |
There was a problem hiding this comment.
Can we change "Any major new" to "Any change in"? I don't want to get into defining "major", and it is more than just "new"
|
|
||
| * Any projects that produce APIs (including CRDs) that are intended to work with kubectl and/or kube-apiserver. (informational - intent is to ensure consistent user experience across the Kubernetes ecosystem) | ||
|
|
||
| * "Critical" or other “highly-integrated” APIs, such as our extension points in the node, apiserver, and controllers as defined by SIG Architecture [TBD]. (mandatory) |
There was a problem hiding this comment.
Can we sort all mandatory before all others?
|
|
||
| * any other github projects in the kubernetes-* orgs (informational, for now - may make mandatory in future) | ||
|
|
||
| * Any projects that produce APIs (including CRDs) that are intended to work with kubectl and/or kube-apiserver. (informational - intent is to ensure consistent user experience across the Kubernetes ecosystem) |
There was a problem hiding this comment.
Any kubernetes-style API that uses a *.k8s.io or *.kubernetes.io name, e.g. "storage.k8s.io" (mandatory)
|
|
||
| As much as possible, we will automate the detection of PRs that require API reviews. Significant changes should be reviewed BEFORE they reach the PR stage, but this is the backstop for anything that gets missed. Such automation may miss cases, so any PR can be flagged as "needs API review", which triggers this process. | ||
|
|
||
| To begin the process, the proposer must produce: |
There was a problem hiding this comment.
suggest to strike "the proposer must produce
|
|
||
| * Create an issue in the kubernetes-sigs/architecture-tracking repository that links to the relevant KEP or documentation | ||
|
|
||
| * The KEPs/documentation should include A clear and thoroughly-researched justification on why the change or addition is needed, including, upgrade/downgrade considerations, and alternatives considered. |
|
|
||
| If **approved**/**reviewed**: | ||
|
|
||
| * Any non-blocking or nit suggestions should be documented in the review document that will be stored in the repository |
There was a problem hiding this comment.
"the review document" is unclear - not defined. Are we keeping a separate doc for API review from the code or KEP PR? Should we require that API be cut-paste into a new PR just for API review? Do we want to keep those forever? Github's historical view is kind of useless for learning (because old state is obliterated).
There was a problem hiding this comment.
I had imagined the doc as something permanent in MD that would be iterated on between the reviewer(s) and submitter(s). If possible, I'd like to try that and see if it works, otherwise, I consider the other options you raise as viable alternatives.
There was a problem hiding this comment.
A code review of a .md file is no different from a code review of a .go file. Github's history is not easy to follow. Just to be clear, you're proposing to create a new markdown file for every API change, which is NOT the actual code, and which we keep forever? Why not just link to the PR containing the proposed changes?
There was a problem hiding this comment.
@thockin I think we can link in the tracking issue to the PR. It's just a little harder to follow after the fact IMO. But this process belongs to the reviewers/approvers, and we can tailor it as necessary over time.
|
|
||
| * Any non-blocking or nit suggestions should be documented in the review document that will be stored in the repository | ||
|
|
||
| * The feedback should be made in the issue with APPROVED or REVIEWED (for externally-maintained CRDs or external components where there is only feedback, not approval) |
There was a problem hiding this comment.
A closed API PR == REJECTED in my estimation. But if the review team sees value in flagging something for reference purposes, I totally understand. I would tend to start without it.
|
|
||
| * Ideally this process should lend itself to some automation. It currently does not. [ TBD: Create an umbrella issue for this ] | ||
|
|
||
| * [We already tag PRs with api-change](https://github.com/kubernetes/test-infra/blob/1611f3be80713f9df933e25e4cf9a2b538302ef5/mungegithub/misc-mungers/deployment/kubernetes/path-label.txt#L7-L17) |
There was a problem hiding this comment.
I want to get rid of mungegithub. This functionality is supported in prow today, it's just that nobody has setup an OWNERS file to implement it. We have (probably undocumented) support for regexes and labeling to accomplish this in one file.
|
|
||
| * [We already tag PRs with api-change](https://github.com/kubernetes/test-infra/blob/1611f3be80713f9df933e25e4cf9a2b538302ef5/mungegithub/misc-mungers/deployment/kubernetes/path-label.txt#L7-L17) | ||
|
|
||
| * We should review whether this can be improved |
There was a problem hiding this comment.
I want to reduce the number of distinct kind/ github labels we use across the org down to 7 +/- 2. Could we consolidate kind/api-change, kind/new-api down to just kind/api-change? Or do we even need a separate kind label for these?
Current org-wide kind/ labels:
- bug
- cleanup
- design
- documentation
- failing-test
- feature
- flake
There was a problem hiding this comment.
I am fine with consolidation.
|
I would suggest adding concrete, albeit aspirational, timelines to the process, e.g: time t: create PR in api-reviews Otherwise it's unclear to submitters how much time to allocate in their project plans for the review process, and also unclear how to gauge whether or not reviewers are meeting realistic timeline goals. Other than that, LGTM. |
|
|
||
| # Process Overview and Motivations | ||
|
|
||
| Due to the importance of preserving usability and consistency in Kubernetes APIs, all changes and additions require expert oversight. The API review process is intended to maintain logical and functional integrity of API implementations over time, the consistency of user experience and the ability of previously written tools to function with new APIs. Wherever possible, the API review process should help change submitters follow [established conventions](https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md), and not simply reject without cause. |
There was a problem hiding this comment.
s/of API implementations/of the API/
|
|
||
| * The proposer may follow one of two paths: | ||
|
|
||
| * Complete the coding of the API change. Create a PR. Request an API review on the PR. (In future, the request will be automated based on detecting API changes). The API reviewer will /approve the PR, assuming the change was satisfactory |
There was a problem hiding this comment.
small: if Clayton, or Brian, or I /approve a PR it is blanket wholly approved. We should not do that. Maybe we should say /lgtm here with final approval when the PR is fully LGTM'ed (in case code or other things are changed)?
|
This is lgtm for the purposes of iteration and I also approve it. I will not tag it until at least two other api approvers provide similar feedback. |
|
thanks Clayton |
|
/lgtm 2 down... |
k8s-ci-robot
added
the
lgtm
|
|
||
| As much as possible, we will automate the detection of PRs that require API reviews. Significant changes should be reviewed BEFORE they reach the PR stage, but this is the backstop for anything that gets missed. Such automation may miss cases, so any PR can be flagged as "needs API review", which triggers this process. | ||
|
|
||
| To begin the process: |
There was a problem hiding this comment.
some overlap with the "Process mechanics" section below, might collapse these as a follow up
|
|
||
| Ideally, reviews will happen as quickly as possible, but it is highly dependent on reviewer availability and bandwidth. In general, the following timeframe can be expected: | ||
|
|
||
| - time t: create PR in api-reviews |
There was a problem hiding this comment.
nit: create issue in kubernetes-sigs/architecture-tracking
|
|
||
| New APIs (groups or Kinds) or substantial changes require KEPs. Major changes without KEPs will be rejected. | ||
|
|
||
| 1. Create an API Review request issue in [https://github.com/kubernetes-sigs/architecture-tracking](https://github.com/kubernetes-sigs/architecture-tracking) - the work will be tracked in the corresponding project board |
There was a problem hiding this comment.
might be helpful to link to the board. https://github.com/kubernetes-sigs/architecture-tracking/projects/3, right?
|
SGTM
…On Mon, Aug 20, 2018 at 1:14 PM Jaice Singer DuMars < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In sig-architecture/api-review-process.md
<#2433 (comment)>:
> +
+ * Complete the coding of the API change. Create a PR. Request an API review on the PR. (In future, the request will be automated based on detecting API changes). The API reviewer will /approve the PR, assuming the change was satisfactory
+
+ * Write the KEP/documentation including a detailed description of the API. Request a review on that. The API reviewer will note in the API review issue the commit SHA at which the KEP was reviewed. The reviewee will note the API review issue number. Later, when the PR is ready to add the API, the reviewee files another review ticket (per above path). The review is expedited because the reviewer only needs to compare the current PR to what was previously approved. This two step process allows for automation, and allows for KEPs to get API approval before moving forward.
+
+* explicit approvals by SIG stakeholders are not checked in the API review process. This is assumed to be given at the PR stage.
+
+* Any additional GitHub IDs associated with the submission (particularly in the case of multiple authors)
+
+* Acknowledgement that they have read and followed the existing API conventions document
+
+## Information to be provided from the reviewer(s)
+
+If **approved**/**reviewed**:
+
+* Any non-blocking or nit suggestions should be documented in the review document that will be stored in the repository
@thockin <https://github.com/thockin> I think we can link in the tracking
issue to the PR. It's just a little harder to follow after the fact IMO.
But this process belongs to the reviewers/approvers, and we can tailor it
as necessary over time.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2433 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFVgVLlUhwLvTJjFsSpQ5rwwy4LYQD2Vks5uSxiSgaJpZM4VlxI2>
.
|
|
@smarterclayton I think the requisite LGTMs are in |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: thockin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
This PR represents a draft Google document which has been LGTM'd by the stakeholder reviewers, as well as reviewed by SIG Architecture.