Skip to content

Promote feature OrderedNamespaceDeletion to GA. #131514

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
Merged
merged 1 commit into from
Jun 10, 2025
Merged

Promote feature OrderedNamespaceDeletion to GA. #131514

merged 1 commit into from
Jun 10, 2025

Conversation

cici37
Copy link
Contributor

@cici37 cici37 commented Apr 28, 2025 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Promote feature OrderedNamespaceDeletion to GA.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 28, 2025
@k8s-ci-robot k8s-ci-robot added area/test sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 28, 2025
BenTheElder
BenTheElder reviewed Apr 28, 2025
View reviewed changes
test/e2e/apimachinery/namespace.go Outdated
@@ -481,7 +480,7 @@ var _ = SIGDescribe("OrderedNamespaceDeletion", func() {
f := framework.NewDefaultFramework("namespacedeletion")
f.NamespacePodSecurityLevel = admissionapi.LevelBaseline

f.It("namespace deletion should delete pod first", feature.OrderedNamespaceDeletion, framework.WithFeatureGate(features.OrderedNamespaceDeletion), func(ctx context.Context) {
Copy link
Member

Choose a reason for hiding this comment

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

We'll keep the WithFeatureGate because it provides information but does not disable running the test in most CI (unless the feature were off-by-default)

but we should drop the e2e feature

https://groups.google.com/a/kubernetes.io/g/dev/c/uVga86nJDuI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review. Updated

@cici37 cici37 mentioned this pull request Apr 28, 2025
Merged
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 28, 2025
@cici37 cici37 mentioned this pull request Apr 28, 2025
Open
8 tasks
@BenTheElder BenTheElder mentioned this pull request Apr 29, 2025
Open
@BenTheElder
Copy link
Member

#131518 merged
/retest

@cici37 cici37 changed the title [WIP]Promote feature OrderedNamespaceDeletion to GA. Promote feature OrderedNamespaceDeletion to GA. Apr 29, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 29, 2025
@cici37
Copy link
Contributor Author

cici37 commented Apr 29, 2025

/assign @jpbetz
for approval. Thanks

@BenTheElder BenTheElder self-assigned this Apr 29, 2025
@siyuanfoundation
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 29, 2025
@jpbetz
Copy link
Contributor

jpbetz commented Apr 30, 2025

Quick request: Can we include a summary of how the GA criteria has been achieved, or if we're not targeting the criteria in the KEP, can we update the KEP to match our target criteria?

@cici37
Copy link
Contributor Author

cici37 commented May 13, 2025

Quick request: Can we include a summary of how the GA criteria has been achieved, or if we're not targeting the criteria in the KEP, can we update the KEP to match our target criteria?

The KEP been updated in kubernetes/enhancements#5191 and ready to merge. Thanks
:)

@jpbetz
Copy link
Contributor

jpbetz commented May 15, 2025

/approve
/hold on kubernetes/enhancements#5191

cc @Jefftree to double check for emu. version adherence

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 15, 2025
@Jefftree
Copy link
Member

For emu version, main thing is checking for disablement tests (in unit/integration) with a previous non locked version. I noticed there aren't any disablement tests with this feature, I'm assuming this is because the existing behavior is already covered by tests and this is intentional? Otherwise emulation version/feature gate changes lgtm.

@BenTheElder
Copy link
Member

I think we want to get the testing going for this so we can aim to add to conformance as well?
#131532

If we can't merge the rest of this soon, I'd request splitting out the test/e2e/apimachinery/namespace.go changes to merge sooner (e2e features => WithFeatureGate) and make sure we have solid CI coverage.

Right now we're not running this test.

@cici37
Copy link
Contributor Author

cici37 commented Jun 3, 2025

@jeffrey Thanks for the review. Yes the existing behavior is covered by test and since it's addressing security concern we'd love to encourage all users to follow the current behavior^^

@BenTheElder
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 3, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: f4b89311e1fdd0db383fdf8cd4c5ef2312d8c43b

@BenTheElder
Copy link
Member

needs rebase

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 4, 2025
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 10, 2025
@k8s-ci-robot k8s-ci-robot requested a review from BenTheElder June 10, 2025 15:45
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cici37, jpbetz

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

@cici37
Copy link
Contributor Author

cici37 commented Jun 10, 2025

/unhold
It's ready to merge :)

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 10, 2025
@BenTheElder
Copy link
Member

/lgtm
straightforward rebase

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 10, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 46994f27737e85de496628dfeb3d507d61a1b88b

@k8s-ci-robot k8s-ci-robot merged commit 3b1178a into kubernetes:master Jun 10, 2025
16 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.34 milestone Jun 10, 2025
@cici37 cici37 deleted the ondGA branch June 10, 2025 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jayunit100 jayunit100 Awaiting requested review from jayunit100

@jpbetz jpbetz Awaiting requested review from jpbetz

@BenTheElder BenTheElder Awaiting requested review from BenTheElder

Assignees

@jpbetz jpbetz

@BenTheElder BenTheElder

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.34
Development

Successfully merging this pull request may close these issues.
6 participants
@cici37 @BenTheElder @siyuanfoundation @jpbetz @Jefftree @k8s-ci-robot