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

Set runtime-config in lockstep with feature-gates, if needed. #1536

Merged
merged 1 commit into from Aug 15, 2023
Merged

Set runtime-config in lockstep with feature-gates, if needed. #1536

merged 1 commit into from Aug 15, 2023

Conversation

benluddy
Copy link
Contributor

@benluddy benluddy commented Aug 11, 2023 •
edited

This supports feature sets containing gates that depend on disabled-by-default API group-versions.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 11, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 11, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 11, 2023
@benluddy benluddy force-pushed the runtime-config branch 4 times, most recently from 268765b to 0edf8e1 Compare August 14, 2023 15:07
@benluddy benluddy changed the title [WIP] runtime-config Set runtime-config in lockstep with feature-gates, if needed. Aug 14, 2023
@benluddy benluddy marked this pull request as ready for review August 14, 2023 15:08
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 14, 2023
benluddy
benluddy commented Aug 14, 2023
View reviewed changes
bindata/assets/config/defaultconfig.yaml
@@ -75,6 +75,7 @@ apiServerArguments:
- StorageObjectInUseProtection
- TaintNodesByCondition
- ValidatingAdmissionWebhook
- ValidatingAdmissionPolicy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The plugin is a no-op unless its feature gate is disabled. Adding it here so that we can enable it for tech preview, which was the motivating use case for this PR.

@benluddy
Copy link
Contributor Author

/test all

dinhxuanvu
dinhxuanvu reviewed Aug 14, 2023
View reviewed changes
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2023
@dinhxuanvu
Copy link
Member

dinhxuanvu commented Aug 14, 2023 •
edited

/hold
Unit test failed.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 14, 2023
@dinhxuanvu
Copy link
Member

Look like the ValidatingAdmissionPolicy is not enabled yet so it run havoc with this test. We can adjust this test for now or just enable this feature in this PR.

@benluddy
Copy link
Contributor Author

benluddy commented Aug 14, 2023 •
edited

/hold Unit test failed. Do we need this PR?

No, I think we need:

  1. Introduce ValidatingAdmissionPolicy gate as disabled. api#1558
  2. bump cluster-config-operator
  3. this change
  4. add ValidatingAdmissionPolicy to tech preview
  5. another cluster-config-operator bump

Look like the ValidatingAdmissionPolicy is not enabled yet so it run havoc with this test. We can adjust this test for now or just enable this feature in this PR.

This change needs to be in before we can enable it, that's the reasoning behind the sequence of changes above.

@benluddy
Copy link
Contributor Author

I'll have to update the test too while the API and cluster-config changes are in progress. We don't want to update test fixtures here every time we add a new feature with special runtime config.

@benluddy
Set runtime-config in lockstep with feature-gates, if needed.
24ccf54 
This supports feature sets containing gates that depend on
disabled-by-default API group-versions.
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2023
@benluddy
Copy link
Contributor Author

/hold

Until cluster-config-operator bump merges to add ValidatingAdmissionPolicy as a known feature.

@benluddy
Copy link
Contributor Author

/hold cancel
/retest-required

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 15, 2023
dinhxuanvu
dinhxuanvu reviewed Aug 15, 2023
View reviewed changes
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 15, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benluddy, dinhxuanvu

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

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 7d5e2e0 and 2 for PR HEAD 24ccf54 in total

@dinhxuanvu
Copy link
Member

/retest-required

1 similar comment
@dinhxuanvu
Copy link
Member

/retest-required

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 15, 2023

@benluddy: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-metal-single-node-live-iso 24ccf54 link false /test e2e-metal-single-node-live-iso
ci/prow/e2e-aws-ovn-single-node 24ccf54 link false /test e2e-aws-ovn-single-node
ci/prow/k8s-e2e-gcp-serial 24ccf54 link false /test k8s-e2e-gcp-serial
ci/prow/e2e-aws-operator-disruptive-single-node 24ccf54 link false /test e2e-aws-operator-disruptive-single-node
ci/prow/e2e-gcp-operator-single-node 24ccf54 link false /test e2e-gcp-operator-single-node

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit 7864ef2 into openshift:master Aug 15, 2023
10 of 15 checks passed
@soltysh soltysh mentioned this pull request Aug 22, 2023
Closed
stbenjam added a commit to stbenjam/cluster-kube-apiserver-operator that referenced this pull request Aug 22, 2023
@stbenjam
Revert "Merge pull request openshift#1536 from benluddy/runtime-config"
04846d9 
This reverts commit 7864ef2, reversing
changes made to 7d5e2e0.
@stbenjam stbenjam mentioned this pull request Aug 22, 2023
Merged
benluddy added a commit to benluddy/cluster-kube-apiserver-operator that referenced this pull request Aug 23, 2023
@benluddy
Revert "Revert "Merge pull request openshift#1536 from benluddy/runti…
189ce0e 
…me-config""

This reverts commit 04846d9.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dinhxuanvu dinhxuanvu dinhxuanvu left review comments

@dgrisonnet dgrisonnet Awaiting requested review from dgrisonnet

@mfojtik mfojtik Awaiting requested review from mfojtik

Assignees

@dinhxuanvu dinhxuanvu

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@benluddy @dinhxuanvu @openshift-ci-robot @openshift-merge-robot