-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
📖 Document our versioning and guarantees in contributing guide #5464
📖 Document our versioning and guarantees in contributing guide #5464
Conversation
/hold for community meeting |
76d4ba0
to
16c7cad
Compare
TODO: define release cadence |
49bb4cd
to
32c1e92
Compare
32c1e92
to
1b4be05
Compare
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.
Overall sounds great. Thx for pushing this forward.
I have a few - mostly clarifying - questions.
6a89e61
to
28ccc5b
Compare
- Compatible API changes like field additions, deprecation notices, etc. | ||
- Breaking API changes for deprecated APIs, fields, or code. | ||
- Features, promotion or removal of feature gates. | ||
- And more! |
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.
Rather than say "And more!" why don't we say something like "everything that may be included in a patch release", to make it clear that the types of changes in minor releases are inclusive of the types of changes in patch releases.
I would also vote to put the description of patch releases first in this document flow, as it's the most common release type, and to use a common presentation mode (I like heading A (*minor*) release CAN include:
followed by a bullet point of change types).
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 the last line on purpose to signal that the list isn't meant to be final or exhaustive, there could be any number of code changes additions etc, see #5464 (comment)
|
||
These guarantees extend to all code exposed in our Go Module, including | ||
*types from dependencies in public APIs*. | ||
Types and functions not in public APIs are not considered part of the guarantee. |
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 you explain a little bit more about this. Are we saying "just because we expose a code object as exportable (i.e., begins with a capital letter) in a library doesn't mean it will never mutate in a future version" ?
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 can rephrase it, this was there before in the contributing guide, I added some context #5464 (comment). Regarding the last line, it refers to unexported code, or everything that's in an internal
package
CONTRIBUTING.md
Outdated
|
||
#### Backporting | ||
|
||
We only accept backports of critical bugs, security issues, or bugs without easy workarounds to the most recent release branch related to a stable API. |
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 we are essentially defining our version support policy as "one minor release". If so, I think we want to make that very clear, perhaps a section for just that (it's an important policy for folks to understand!)
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.
It's more than just one minor, it's the set of minor releases related to an API version as defined in the previous section, how would you like me to reword it? (I agree it might be confusing)
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.
Yeah, it's hard. If we're already stating that in the previous section, can we simply omit the "to the most recent release branch related to a stable API" part? If we want to be super explicit about how this works, we might need to provide some concrete examples.
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.
Yep that works for me
This changeset also completely removes the outdated VERSIONING.md document. Signed-off-by: Vince Prignano <[email protected]>
28ccc5b
to
dc20e3d
Compare
/lgtm |
| **v1alpha3** | release-0.3 | 2022-02-23 | | ||
|
||
- The API version is determined from the GroupVersion defined in the top-level `api/` package. | ||
- The EOL date is determined from the last release available once a new API version is published. |
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.
Is there already a rule how the EOL is calculated, i.e. how long after "the last release available once a new API version is published"?
But we don't have to solve that on this 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.
We haven't calculated EOL dates before, this is a new rule that we ~discussed in one of the last community meetings. We're doing 6 months on the above because of alpha APIs, but we might want to do longer given the promotion to beta.
/lgtm I agree with Fabrizio, it's a huge improvement. There will always be edge cases and we can clarify them later on or decide on a case-by-case basis. |
/hold cancel |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon 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 |
This changeset also completely removes the outdated VERSIONING.md
document.
Signed-off-by: Vince Prignano [email protected]
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #5459