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.

Sign up for GitHub

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

Jump to bottom

KEP-Template: Clarify the stage name of deprecated feature gates #4898

Merged
merged 1 commit into from
Jan 17, 2025
Merged

KEP-Template: Clarify the stage name of deprecated feature gates #4898

merged 1 commit into from
Jan 17, 2025

Conversation

HirazawaUi
Copy link
Contributor

@HirazawaUi HirazawaUi commented Oct 2, 2024 •
edited
Loading

  • One-line PR description: Clarify the stage name of deprecated feature gates
  • Issue link:

For us, deprecating a user-visible feature, or adding a bug fix that could be seen as 'removal', requires a small KEP for discussion and to document them and we also should add a Deprecated feature gate for such cases.

For these Deprecated feature gates, their lifecycle is shorter than a full feature gate lifecycle, typically following featuregate.Deprecated → wait a few releases → remove. So, we only need two phases to describe them, but we haven't clearly defined the names of these phases yet.

Some useful references: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/feature-gates.md#:~:text=Bug%20fixes%20that,can%20be%20disabled.

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/feature-gates.md#deprecation

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 2, 2024
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 2, 2024
@HirazawaUi
Copy link
Contributor Author

/cc @thockin @tengqm @wojtek-t @liggitt @jpbetz

@HirazawaUi HirazawaUi force-pushed the add-deprecation-stage branch from 8cbc810 to c271cf2 Compare October 15, 2024 17:06
@k8s-ci-robot k8s-ci-robot added area/enhancements Issues or PRs related to the Enhancements subproject kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 15, 2024
HirazawaUi
HirazawaUi commented Oct 15, 2024
View reviewed changes
api/approval.go Outdated
Stable *PRRMilestone `json:"stable" yaml:"stable,omitempty"`
Deprecated *PRRMilestone `json:"deprecated" yaml:"deprecated,omitempty"`
Removed *PRRMilestone `json:"removed" yaml:"removed,omitempty"`
DeprecatedDisabled *PRRMilestone `json:"deprecatedDisabled" yaml:"deprecated-disabled,omitempty"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding fields here is currently unused, but Removed and Deprecated fields have already been added here, and they also not be referenced by any code.

@mrbobbytables mrbobbytables removed their request for review October 15, 2024 17:30
@k8s-triage-robot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

thockin
thockin reviewed Nov 26, 2024
View reviewed changes
api/approval.go Outdated
Stable *PRRMilestone `json:"stable" yaml:"stable,omitempty"`
Deprecated *PRRMilestone `json:"deprecated" yaml:"deprecated,omitempty"`
Removed *PRRMilestone `json:"removed" yaml:"removed,omitempty"`
DeprecatedDisabled *PRRMilestone `json:"deprecatedDisabled" yaml:"deprecated-disabled,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Super nit-pick, but maybe we want "deprecatedAnnounced" and "deprecatedDisabled" for congruence?

Copy link
Member

Choose a reason for hiding this comment

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

Or deprecated->disabled->removed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For compatibility reasons, I think deprecated->disabled->removed is better, because some KEPs already use Deprecated as milestone, and changing it to deprecatedAnnounced might confuse some users!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thockin Do you think it's satisfactory enough at present?

@HirazawaUi HirazawaUi force-pushed the add-deprecation-stage branch from c271cf2 to eec531d Compare December 16, 2024 14:31
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 16, 2024
@thockin
Copy link
Member

thockin commented Jan 17, 2025

Thanks!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2025
@thockin thockin mentioned this pull request Jan 17, 2025
Merged
@johnbelamaric
Copy link
Member

/approve
/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HirazawaUi, johnbelamaric, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 17, 2025
@k8s-ci-robot k8s-ci-robot merged commit b4a745e into kubernetes:master Jan 17, 2025
4 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.33 milestone Jan 17, 2025
@vinayakankugoyal vinayakankugoyal mentioned this pull request Jan 17, 2025
Merged
@vinayakankugoyal
Copy link
Contributor

Hi folks,

I think this PR missed updating the validation to support the new stages. I made an attempt to fix that here: #5049

@HirazawaUi HirazawaUi mentioned this pull request Jan 22, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thockin thockin thockin left review comments

@jeremyrickard jeremyrickard Awaiting requested review from jeremyrickard

@jpbetz jpbetz Awaiting requested review from jpbetz

@liggitt liggitt Awaiting requested review from liggitt

@tengqm tengqm Awaiting requested review from tengqm

@wojtek-t wojtek-t Awaiting requested review from wojtek-t

Assignees

@thockin thockin

@johnbelamaric johnbelamaric

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/enhancements Issues or PRs related to the Enhancements subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.33
Development

Successfully merging this pull request may close these issues.
6 participants
@HirazawaUi @k8s-triage-robot @thockin @johnbelamaric @k8s-ci-robot @vinayakankugoyal