Add GA API policy and describe feature gates #5634
Conversation
|
Skipping CI for Draft Pull Request. |
tekton-robot
added
release-note
tekton-robot
added
the
size/M
|
/kind misc |
tekton-robot
added
the
kind/misc
|
@dibyom is this meant to clarify the existing api policy, or to propose the api policy discussed in this week's WG and your API versioning doc? (or is this just not ready for review yet? :) ) |
|
All 3, but mostly not ready for review yet :) Discovered some new issues yesterday #5628 (comment) |
tekton-robot
added
size/L
dibyom
changed the title
tekton-robot
removed
the
do-not-merge/work-in-progress
|
@tektoncd/core-maintainers PTAL! |
api_compatibility_policy.md
Outdated
|
|
||
| - We will not make any backwards incompatible changes to fields that are stable without incrementing the api version. | ||
|
|
||
| - Stable APIs do not follow the [Go libraries compatibility policy](#go-libraries-compatibility-policy) i.e. backwards incompatible changes to the Go struct is not allowed. |
There was a problem hiding this comment.
this is a bit confusing since the go libraries policy is more about software versioning than API versioning. It seems weird to say that our go libraries compat policy depends on the part of the codebase. Maybe we could say that after we release software version 1.0, we won't make any backwards incompatible changes to exported functions?
There was a problem hiding this comment.
Hmm, I was thinking of only a subset of the go packages - esp. the ones in pkg/api/v1 and pkg/client. I'm not sure we need to have all exported functions under this
There was a problem hiding this comment.
@wlynch for #2748 (comment) was your comment around deprecation policies for all exported functions or the ones in the pkg/apis/pipeline/v1 package?
There was a problem hiding this comment.
Honestly maybe everything that's not in apis should be in an internal package?
There was a problem hiding this comment.
Honestly maybe everything that's not in apis should be in an internal package?
In theory yes 😬. Like in theory most of what is in pkg today should be in internal/pkg or something unless required otherwise (like pkg/apis, the generated clients, …). In practice using internal is not always super known by go dev.
There was a problem hiding this comment.
I agree with Billy that things indexed on pkg.go.dev make sense to cover in our compatibility policy. I would find it confusing to have a compatibility policy that covers some of our public go packages but not others. I agree we don't want to promise no changes to pkg/, but we could say we won't remove/rename a deprecated function/const for x time?
There was a problem hiding this comment.
we could say we won't remove/rename a deprecated function/const for x time?
We could perhaps provide that but backwards compatibility technically also includes things like adding new arguments or return values to a function: https://go.dev/blog/module-compatibility
@wlynch I'm not sure how we can both guarantee go module compatibility while also making rapid changes to alpha APIs if all of pkg/ is under scope.
I think clearly stating out which packages are in scope and which are not is a decent middle ground. Over time, we could move the the out of scope packages into internal. Another option would be to clearly write out what subset of compatibility we can provide e.g. we can support not deprecating/removing functions but we'd probably want the ability modify function signatures in most of pkg/
There was a problem hiding this comment.
I'm a bit confused about how the policy can be different for stable APIs only - as go libraries are not necessarily bound to a specific API version.
Does this summary make sense:
- go APIs specific to stable Tekton APIs are subject to the new policy, i.e. we cannot do backwards incompatible changes. Also, a future
v2public API must be separated in code from thev1public API - go APIs specific to alpha/beta Tekton APIs can be subject to backwards-incompatible changes
- go APIs which are shared between alpha/beta and stable that needs to stay public would have to be separated, one copy under the stable tree, one copy under the alpha/beta one
- go APIs that we don't need to be public would have to become private, and we could (1) move them to private (2) create a public shim (3) deprecate the shim right away. This would have to happen before v1
There was a problem hiding this comment.
The first two points make sense to me.
-
For public go apis shared between alpha and beta - why do we need to copy unless we are making a breaking change? There is quite a bit of complexity involved in this so I'm sure we should do this unless we really need to
-
I support moving them to private incrementally but I don't think we need to block the v1 API release on this. I'm unsure why we'd need to deprecate the shim.
We had some discussion on this in the last API WG - the conclusion was that I'd update the policy to mention add guidelines for making changes to the go libraries. Specifically this would be to avoid making backwards incompatible changes to the go APIs pertaining to the V1 API version.
There was a problem hiding this comment.
The first two points make sense to me.
- For public go apis shared between alpha and beta - why do we need to copy unless we are making a breaking change? There is quite a bit of complexity involved in this so I'm sure we should do this unless we really need to
I meant shared between alpha/beta and stable. If we can make sure (through testing) that we would catch those cases, then it would be fine to not copy the code until a breaking change is needed on alpha or beta side.
- I support moving them to private incrementally but I don't think we need to block the v1 API release on this. I'm unsure why we'd need to deprecate the shim.
We don't need to block v1 only on this as long as our API stability policy excludes them upfront.
If we create a shim, it would only be to give existing users more time to move away from those APIs that we want to become private. But we would have to deprecate the shim to signal our users that that is only a temporary solution.
Alternatively we could deprecate the API before making it private, and when it moves to private provide no shim at all.
We had some discussion on this in the last API WG - the conclusion was that I'd update the policy to mention add guidelines for making changes to the go libraries. Specifically this would be to avoid making backwards incompatible changes to the go APIs pertaining to the V1 API version.
dibyom
changed the title
This commit updates the Go library compatibility policy with guidelines for Go client libraries for a particular CRD version. The guidelines are based on the discussions in tektoncd#5634, tektoncd#2748, and in the API WG on Oct 10, 2022. Co-authored-by: Lee Bernick <[email protected]> Signed-off-by: Dibyo Mukherjee <[email protected]>
|
I updated and split up the PR to separate out the bit around go libraries compatibility since that seems to be more controversial. Happy to move forward without the changes to Go libraries if that is easier! @vdemeester @lbernick Let me know if the update captures the discussion and what you had in mind Also, @afrittoli and any other @tektoncd/core-maintainers this should be ready for another review! |
This commit updates the Go library compatibility policy with guidelines for Go client libraries for a particular CRD version. The guidelines are based on the discussions in tektoncd#5634, tektoncd#2748, and in the API WG on Oct 10, 2022. Co-authored-by: Lee Bernick <[email protected]> Signed-off-by: Dibyo Mukherjee <[email protected]>
|
@vdemeester @afrittoli @lbernick ready for another look 🙏 |
tekton-robot
added
the
approved
api_compatibility_policy.md
Outdated
|
|
||
| - Backwards incompatible changes in `pkg/apis/pipeline/v1beta1` (or another beta API) require providing a notice of the upcoming change for at least 3 releases before making the change. However, they do not require a major version bump. | ||
|
|
||
| - For `pkg/apis/pipeline/v1` (or any other stable APIs), any [backwards incompatible change](https://go.dev/blog/module-compatibility) will require a major version bump. |
There was a problem hiding this comment.
By major we mean {major}.{minor}.{bugfix} ?
There was a problem hiding this comment.
I think it refers to the API version e.g. v1 -> v2, that is, backwards incompatible changes on that package cannot be done, they must go in a new package
There was a problem hiding this comment.
I think this was intended to refer to semver since it's talking about the go libraries-- @dibyom can you update to clarify?
There was a problem hiding this comment.
I was referring to semver. I do think we need to bump the API version if we make a breaking change to the API itself, but if we are just renaming a function or something, I think we can just update the semver version.
There was a problem hiding this comment.
I'm confused. We have the API version, which is not semver, and the version of the go module, for which we only have used minor and patch, major is still 0 (e.g. 0.41.1). So we can do a breaking change in the code in pkg/apis/pipeline/v1, as long as we release a v1.0.0, v2.0.0 etc?
About renaming, for an API in a package associated to a stable version, I would prefer we never did it in one go, regardless of the version. We should create a new field / function with the name we want and deprecate the old one.
There was a problem hiding this comment.
So we can do a breaking change in the code in pkg/apis/pipeline/v1, as long as we release a v1.0.0, v2.0.0 etc?
Technically yes, the intent is to strongly discourage these kinds of changes since major version bumps should be rare. (If it changes the user facing API and not just a function, we'd need to change the API version as well)
About renaming, for an API in a package associated to a stable version, I would prefer we never did it in one go, regardless of the version. We should create a new field / function with the name we want and deprecate the old one.
I agree, I don't think anything here is saying we should not do that. (I can include that explicitly if that helps)
This commit updates the Go library compatibility policy with guidelines for Go client libraries for a particular CRD version. The guidelines are based on the discussions in tektoncd#5634, tektoncd#2748, and in the API WG on Oct 10, 2022. Co-authored-by: Lee Bernick <[email protected]> Signed-off-by: Dibyo Mukherjee <[email protected]>
|
/hold - need to fix a commit |
tekton-robot
added
the
do-not-merge/hold
This commit updates the Go library compatibility policy with guidelines for Go client libraries for a particular CRD version. The guidelines are based on the discussions in tektoncd#5634, tektoncd#2748, and in the API WG on Oct 10, 2022. Co-authored-by: Lee Bernick <[email protected]> Signed-off-by: Dibyo Mukherjee <[email protected]>
|
/hold cancel |
tekton-robot
removed
the
do-not-merge/hold
dibyom
changed the title
This commit describes our API policiy for our GA APIs (e.g. v1). It also clarifies how TEP-33 Feature Gates interacts with CRD API versions. Fixes tektoncd#5633 Signed-off-by: Dibyo Mukherjee <[email protected]>
|
[Splitting off the discussion around Go libraries and releases to a separate PR] |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afrittoli, lbernick, vdemeester 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 |
|
/lgtm |
Changes
Add GA API policy and describe feature gates
This commit describes our API policiy for our GA APIs (e.g. v1). It also
clarifies how TEP-33 Feature Gates interacts with CRD API versions.
Fixes #5633
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes