add proposal for a process for api-reviews #221
Conversation
kubevirt-bot
added
the
dco-signoff: no
Signed-off-by: Alay Patel <[email protected]>
alaypatel07
force-pushed
the
api-review-proposal
branch
from
d7a8007 to
85eb1e4
Compare
kubevirt-bot
added
dco-signoff: yes
|
cc @rthallisey |
design-proposals/api-review.md
Outdated
| - Contributors | ||
|
|
||
| ## User Stories | ||
| - As a user of kubevirt project, how can I feel confident that kubevirt maintains an intuitive, stable and crisp API that |
There was a problem hiding this comment.
I wouldn't have user stories in question form.
-As a user of kubevirt project, how can I feel confident that kubevirt maintains an intuitive, stable and crisp API that can be used as a foundational block to build products and projects.
+As a user of kubevirt project, I want to feel confident that kubevirt maintains an intuitive, stable and API that can be used as a foundational block to build products and projects.-As a contributor, how do I know the right way to approach API facing change?
+As a contributor, I need guidance on the right way to approach API facing changeect...
design-proposals/api-review.md
Outdated
| In order to achieve the stability, quality and consistency of APIs like the core Kubernetes APIs this document proposes | ||
| the following changes: | ||
|
|
||
| - Process for api reviewers: Process changes to add more resources and visibility for api reviews |
There was a problem hiding this comment.
I would express it this way:
- There should be a one or more engineers that review api breaking changes on a regular basis.
- There should be a guide explaining how to merge and api change
- Tools and tests:...
There was a problem hiding this comment.
Accommodated the feedback in the language. I have kept a title prefix to each point so it can be reference point for heading to follow later in section
Signed-off-by: Alay Patel <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Co-authored-by: Dan Kenigsberg <[email protected]> Signed-off-by: Alay Patel <[email protected]>
alaypatel07
force-pushed
the
api-review-proposal
branch
from
c411a34 to
951a904
Compare
|
@dankenigsberg thanks for the review, accommodated all your suggestions! |
|
/cc |
Signed-off-by: Alay Patel <[email protected]>
|
@stu-gott I have addressed all the suggestions, PTAL |
|
/cc |
|
/cc |
Signed-off-by: Alay Patel <[email protected]>
There was a problem hiding this comment.
Thank you for the proposal.
This is a "hot" topic, especially when we find it challenging to maintain so many features with still-undefined policy of how a feature/api lifecycle should look like.
From my side, I will like to make sure we have freedom to experiment in alpha and beta stages without strong commitment to keep the features/apis in place.
| Since KubeVirt has reached v1 and has some APIs with v1 version, implementing an API review process | ||
| ensures code quality, usability, stability, and fosters community collaboration, ultimately leading to a successful and | ||
| sustainable software ecosystem. |
There was a problem hiding this comment.
This seems to describe what an API in general provides, i.e. a contract.
What is not very clear to me is what is missing or what needs to be solved. I think it is important to give a good reasoning so we can remind ourself the missing part and their implications.
There was a problem hiding this comment.
My intent was to describe the missing parts in the rest of the document. Hence, I have added a statement that the focus-areas where improvement is needed is described in the goals section and how to go about those focus areas in design section.
| - Establish a process for API reviews such that: | ||
| - contributors have a clear idea about how to implement an API facing change | ||
| - reviews have a guideline about the necessary checks required for a successful API facing change | ||
| - community has a guideline about how to handle API breakages upon upgrade |
There was a problem hiding this comment.
An effort has started to define a feature-lifecycle, discussing the different stages features go through, including API introduction and changes.
I guess that work should eventually sync with any ideas presented here.
design-proposals/api-review.md
Outdated
| - Contributors | ||
|
|
||
| ## User Stories | ||
| - As a user of KubeVirt project, I want KubeVirt to maintain an intuitive, stable and simple API, that can |
There was a problem hiding this comment.
I think this user is not defined above.
| ## Process for api reviewers | ||
| Recent changes to reviewer guidelines recommend forming small groups in specific areas of expertise. sig-api is | ||
| one such group. This group will be responsible for: | ||
| - Reviewing all the PRs with `kind/api-change` labels |
There was a problem hiding this comment.
This is a major change in responsibility and ownership.
While I am in favor of such cross-sigs ownership, it still needs to be accepted by all sig maintainers, as it will effect velocity of development.
E.g. if the SIG will not have the capacity to review the changes, will it block the PR?
There was a problem hiding this comment.
In order to make sure that what effect this will have, the implementation is staggered into phases. Phase 1 is where most of the pains of this new process will be evident. In order to solve for the pain, automation tools can be introduced in phase 2 and phase 3. With automation and the best practices guide, I think we can cater to the needed velocity of development.
| - Any other communication that is needed for contributor to move forward. | ||
| - While goal of the project is to prohibit introducing API breaking changes, automation tools proposed in the Tools section | ||
| will help in attaining this goal. However, some APIs have reached v1 (GA) without any such tool. Hence, a well-defined | ||
| process is needed to address breaking changes discovered in previous versions: |
There was a problem hiding this comment.
I think this needs to be defined with their rules set as part of the feature-lifecycle.
Exceptions could be discussed in such a sig-api, but changes to the API are in some cases fine, e.g. alpha features and their API/s.
There have been discussions on the technicalities of alpha fields which can be removed, are these details going to enter in this proposal or some appendix?
There was a problem hiding this comment.
I have added the conclusion of the discussions about alpha fields in Policy of API Evolution Section
There have been discussions on the technicalities of alpha fields which can be removed, are these details going to enter in this proposal or some appendix?
In general this topic relates to a change that is in a GA field which is not supposed to break backward compatibility, but breaks compat due to oversight, how does the community react to that? The following lines provide a guideline for dealing with that.
| - Have a document to describe the right process for contributors with the following details: | ||
| - Any API-facing change with design doc can be reviewed by the sig-api for initial feedback. | ||
| - Any API-facing change PR can be brought up for the discussion in sig-api call | ||
| - Link to a conventions document for good practices and guidance |
There was a problem hiding this comment.
Is this going to be part of the current proposal?
There was a problem hiding this comment.
no, it wasnt intended to be added in this proposal. This will be once the sig has collected enough good practices, I can start seeding such a document
design-proposals/api-review.md
Outdated
| (an overview on the approaches used to functional test this design) | ||
|
|
||
| # Implementation Phases | ||
| (How/if this design will get broken up into multiple phases) |
There was a problem hiding this comment.
It will be nice to fill this up.
I guess starting the SIG formally has already occurred, starting reviewing PR/s, working on the guidance document and possibly giving a talk to help contributors and reviewers to follow it.
There was a problem hiding this comment.
added Implementation Phases, PTAL
|
/assign |
|
New changes are detected. LGTM label has been removed. |
This policy describes how alpha features can evolve with stable, beta or alpha API Signed-off-by: Alay Patel <[email protected]>
alaypatel07
force-pushed
the
api-review-proposal
branch
from
d6ae9ec to
cce2a53
Compare
| - Contributors | ||
|
|
||
| ## User Stories | ||
| - As a stakeholder of KubeVirt project, I would like the project to have a crip policy of how APIs will evolve across |
|
|
||
| ## Policy of API Evolution | ||
| - Alpha features should always introduce an optional field to APIs that have GA'ed or in Beta | ||
| - Alpha features and hence by extensions fields that are introduced behind a feature flag, is not guaranteed to work |
There was a problem hiding this comment.
| - Alpha features and hence by extensions fields that are introduced behind a feature flag, is not guaranteed to work | |
| - Alpha features and hence by extensions fields that are introduced behind a feature flag, are not guaranteed to work |
| work is not slowed down by requirement to maintain api-compatibility. Even though compatibility for experimental or | ||
| alpha APIs is not strictly required, but breaking compatibility should not be done lightly, as it disrupts all users | ||
| of the feature. | ||
| - Beta or GA features that introduces new fields to beta or stable APIs should not break compatibility |
There was a problem hiding this comment.
Is this really achieveable for our project? It sounds like a bold commitment.
There was a problem hiding this comment.
We really need to aspire for this goal and make tools/processes that can help make it achievable. It's hard but not impossible. Following are some of the reasons why this is needed:
- KubeVirt has reached v1. There are several users deploying large scale production workloads using KubeVirt
- KubeVirt does not currently support downgrades. For users that have deployed an older version, if the upgrade breaks workloads, then there is no way for users to proceed ahead. Absolute care must be taking for Beta and GA APIs to not break upon upgrades.
This proposal talk about some of the tools and processes to make this achievable:
- process changes where a SIG owns API facing changes that goes into releases will really help make this a reality. This sig will collate the knowledge and provide best practices/guidance/policy docs to ensure backward compatibility.
- automated tools like fuzzer tests https://github.com/alaypatel07/kubevirt-api-fuzzer#usage will help make sure that GA and beta APIs are round trip-able across version upgrades
tagging @xpivarc @EdDev @rthallisey if you guys have additional thoughts on the above.
Signed-off-by: Alay Patel <[email protected]>
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with /lifecycle stale |
kubevirt-bot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with /lifecycle rotten |
kubevirt-bot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. /close |
|
@kubevirt-bot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
This adds a design document to address
API change review guidelinesbullet point in kubevirt/kubevirt#8566.At a high level, the process proposes 4 major improvements: