Skip to content
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

RFC: Update Go compatibility policy #6768

Merged
merged 1 commit into from
Jun 20, 2023
Merged

Conversation

lbernick
Copy link
Member

@lbernick lbernick commented Jun 2, 2023

This commit strengthens the stability policy for Tekton Pipelines' Go libraries for stable APIs, while allowing us to continue to evolve alpha APIs (and find a middle ground for beta APIs).

Goals are:

  • maximize consistency
  • minimize confusion
  • clarify future work that needs to be done

This inevitably involves some judgment calls that can be revisited over time.

For a helpful reference on how Kubernetes handles unstable fields within stable APIs, see https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#adding-unstable-features-to-stable-versions. This is a helpful reference, but not all of these guidelines are applicable to Tekton given its current compatibility policies.

/kind misc
Closes #2748

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

  • Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
  • n/a Has Tests included if any functionality added or changed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including functionality, content, code)
  • Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
  • n/a Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
  • n/a Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

@tekton-robot tekton-robot added the release-note-none Denotes a PR that doesnt merit a release note. label Jun 2, 2023
@tekton-robot tekton-robot added kind/misc Categorizes issue or PR as a miscellaneuous one. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 2, 2023
Comment on lines 191 to 192
- If support for an alpha field is dropped, the field will be tombstoned in client libraries and will not be removed.

- If backwards incompatible changes are made to an alpha or beta field after the warning period, the same changes will be made to the client libraries at that time.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These parts I'm a bit unsure of but it seemed like the best way to achieve consistency with our policies on alpha/beta features-- open to suggestions.

@lbernick
Copy link
Member Author

lbernick commented Jun 2, 2023

@tektoncd/core-maintainers I've directly assigned folks who have been more active in previous conversations on Go compat policies, but feedback is welcome from everyone!

@jerop jerop added this to the Pipelines v0.49 milestone Jun 5, 2023
api_compatibility_policy.md Outdated Show resolved Hide resolved
When it is complete, the compatibility policy will be updated to include all of `pkg/apis`.

Pipelines contributors also plan to move all internal functionality in the entire [github.com/tektoncd/pipeline Go package](https://pkg.go.dev/github.com/tektoncd/pipeline)
into internal packages. This work is tracked in [issue #5679](https://github.com/tektoncd/pipeline/issues/5679).
Copy link
Member

@Yongxuanzhang Yongxuanzhang Jun 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR. But for the #5679, it seems that we haven't reached an alignment. I'm worried about the disadvantages we could get from it outweigh the benefits. It's hard to find out what functions should be internal, and it also limits the growth of tekton ecosystem by disallowing the usages of current public functions

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback! I retitled the issue. I think the purpose of the issue is to move functions that we don't want to expose externally into internal packages, so that it's possible to have a compat policy for everything else. It seems like the controversial part is what should be internal and what shouldn't be, but there was general consensus on moving internal functionality into internal packages. I agree this is challenging, but it seems to me like a good long-term goal.

Comment on lines 173 to 177
- If support for an alpha CRD is dropped, its client library will not be removed until the last release with support for the CRD has reached its end of life.

- If support for a field is dropped, the field may also be removed from the client libraries; i.e. it is not guaranteed to be tombstoned.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it mean to "tombstone" a field? I can guess but I'm not entirely sure and I couldn't easily find a definition online. Perhaps it makes sense to link to a definition?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a definition here, lmk if it is clear!

@lbernick
Copy link
Member Author

lbernick commented Jun 7, 2023

/retest
#6772

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2023
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2023
@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 13, 2023
@@ -74,3 +74,8 @@ how these affect your deprecation or timeline, ask the maintainers.
versions. Once you are reasonably confident that the field is no longer used
by supported servers (rule of thumb: target currently supported LTS servers),
it can safely be removed from client types.

4. Maintain backwards compatibility of Go libraries when possible.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯


- Go structs representing stable fields may not change in breaking ways without a semver major version bump, even if the yaml/JSON API is unaffected.

- If support for a stable CRD is dropped (via a semver major version bump), the client libraries for this CRD will continue to be supported.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "supported" means in this context?
Does it mean we provide security patches, updates for newer versions of k8s etc?
How are we going to generate the clients for updates if the CRD is not there? Are we going to keep the CRD around and not serve it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the feedback-- clarified that "dropping" means "not serving" and "supporting" means keeping the client libraries preserved, but I don't think we would need to do anything like security patches.

This commit strengthens the stability policy for Tekton Pipelines' Go libraries for stable APIs,
while allowing us to continue to evolve alpha APIs (and find a middle ground for beta APIs).

Goals are:
- maximize consistency
- minimize confusion
- clarify future work that needs to be done

This inevitably involves some judgment calls that can be revisited over time.

For a helpful reference on how Kubernetes handles unstable fields within stable APIs, see
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#adding-unstable-features-to-stable-versions.
This is a helpful reference, but not all of these guidelines are applicable to Tekton given its current compatibility policies.
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates!
/approve

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, JeromeJu, vdemeester, Yongxuanzhang

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:
  • OWNERS [afrittoli,vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vdemeester
Copy link
Member

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 20, 2023
@tekton-robot tekton-robot merged commit b4f0543 into tektoncd:main Jun 20, 2023
@lbernick lbernick mentioned this pull request Jun 29, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/misc Categorizes issue or PR as a miscellaneuous one. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesnt merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define compatibility policy for Go libs
9 participants