-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
update the --runtime-config handling to ensure that user preferences always take priority over hardcoded preferences #108029
Conversation
runtimeConfig: map[string]string{ | ||
"apps/v1/deployments": "false", | ||
}, | ||
defaultResourceConfig: func() *serverstore.ResourceConfig { | ||
return newFakeAPIResourceConfigSource() | ||
}, | ||
expectedAPIConfig: func() *serverstore.ResourceConfig { | ||
return newFakeAPIResourceConfigSource() | ||
config := newFakeAPIResourceConfigSource() | ||
config.DisableResources(appsv1.SchemeGroupVersion.WithResource("deployments")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now allowed since we're going to need it to enable particular beta APIs.
@@ -132,13 +139,16 @@ func TestParseRuntimeConfig(t *testing.T) { | |||
}, | |||
expectedAPIConfig: func() *serverstore.ResourceConfig { | |||
config := newFakeAPIResourceConfigSource() | |||
config.DisableVersions(appsv1.SchemeGroupVersion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this API group/version was added to allow new unit testing below.
continue | ||
} | ||
|
||
tokens := strings.Split(key, "/") | ||
if len(tokens) < 2 { | ||
if len(tokens) < 2 || len(tokens) > 3 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking: should we warn we're ignoring these keys?
for _, versionPreference := range versionPreferences { | ||
// if a user has expressed a preference about a version, that preference takes priority over the hardcoded resources | ||
resourceConfig.RemoveMatchingResourcePreferences(resourceMatcherForVersion(versionPreference.groupVersion)) | ||
if versionPreference.enabled { | ||
// enable the groupVersion for "group/version=true" and "group/version/resource=true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the and "group/version/resource=true"
portion of this comment no longer applies here
} else if len(tokens) == 2 { | ||
resourceConfig.EnableVersions(versionPreference.groupVersion) | ||
|
||
} else { | ||
// disable the groupVersion only for "group/version=false", not "group/version/resource=false" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the not "group/version/resource=false"
portion of this comment no longer applies here
…always take priority over hardcoded preferences
edf68f3
to
58e9567
Compare
/approve verify failure is legitimate (unused var) and a nit on import order |
appsv1 "k8s.io/api/apps/v1" | ||
|
||
"k8s.io/apimachinery/pkg/runtime/schema" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: group with k8s imports
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, liggitt 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 |
58e9567
to
41b2662
Compare
/kind feature |
/priority important-soon |
/lgtm |
/triage accepted |
This is in preparation for the "beta APIs are off by default" implementation. To support that, we need an order of precedence that looks like
This is done by removing the per-resource preferences specified by the default when a stability level or version is specified by the user that overlaps.