-
Notifications
You must be signed in to change notification settings - Fork 557
OCPNODE-3372: Update defaultRuntime doc to show options and set default to crun #2370
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
base: master
Are you sure you want to change the base?
Changes from all commits
c3bedd4
14c1130
944efda
e6104a2
011f4ca
1255bfb
a3b721e
d339a49
0d729b6
01d83a6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,146 @@ | ||
apiVersion: apiextensions.k8s.io/v1 # Hack because controller-gen complains if we don't have this | ||
name: "ContainerRuntimeConfig" | ||
crdName: containerruntimeconfigs.machineconfiguration.openshift.io | ||
tests: | ||
onCreate: | ||
- name: Should fail if set to empty string | ||
initial: | | ||
apiVersion: machineconfiguration.openshift.io/v1 | ||
kind: ContainerRuntimeConfig | ||
spec: | ||
containerRuntimeConfig: | ||
defaultRuntime: "" | ||
expectedError: "spec.containerRuntimeConfig.defaultRuntime: Unsupported value: \"\": supported values: \"crun\", \"runc\"" | ||
- name: Should not fail if not set and other fields set | ||
initial: | | ||
apiVersion: machineconfiguration.openshift.io/v1 | ||
kind: ContainerRuntimeConfig | ||
spec: | ||
containerRuntimeConfig: | ||
logLevel: info | ||
expected: | | ||
apiVersion: machineconfiguration.openshift.io/v1 | ||
kind: ContainerRuntimeConfig | ||
spec: | ||
containerRuntimeConfig: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @everettraven I've reverted the omitempty removal and kept defaultRuntime as a non-pointer type. My reasoning is:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Without a pointer, MCO cannot tell the difference between unset and the empty string. Does the empty string mean anything to crun?
This is called discoverability in our APIs. When a controller generates a configuration API object, we try to generate a complete set of fields so that a user can easily see from the object, where there are options they may not have known about (they can discover the new options).
But it won't round trip. As soon as a structured client writes the object, the field will be removed from etcd and future reads will not render the Either remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @JoelSpeed . I have removed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @JoelSpeed can you review? Happy to address any comments you have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are unstructured calls, unless there's an explicit default, I wouldn't expect the |
||
logLevel: info | ||
- name: Should fail if set to a number | ||
initial: | | ||
apiVersion: machineconfiguration.openshift.io/v1 | ||
kind: ContainerRuntimeConfig | ||
spec: | ||
containerRuntimeConfig: | ||
defaultRuntime: 1234 | ||
expectedError: "spec.containerRuntimeConfig.defaultRuntime: Invalid value: \"integer\": spec.containerRuntimeConfig.defaultRuntime in body must be of type string: \"integer\", spec.containerRuntimeConfig.defaultRuntime: Unsupported value: 1234: supported values: \"crun\", \"runc\"" | ||
- name: Should fail if set to a blank string | ||
initial: | | ||
apiVersion: machineconfiguration.openshift.io/v1 | ||
kind: ContainerRuntimeConfig | ||
spec: | ||
containerRuntimeConfig: | ||
defaultRuntime: " " | ||
expectedError: "Unsupported value: \" \": supported values: \"crun\", \"runc\"" | ||
- name: Should fail if invalid ContainerRuntimeConfig is provided | ||
initial: | | ||
apiVersion: machineconfiguration.openshift.io/v1 | ||
kind: ContainerRuntimeConfig | ||
spec: | ||
containerRuntimeConfig: | ||
defaultRuntime: docker | ||
expectedError: "Unsupported value: \"docker\": supported values: \"crun\", \"runc\"" | ||
- name: Should be able to set crun in ContainerRuntimeConfig | ||
initial: | | ||
apiVersion: machineconfiguration.openshift.io/v1 | ||
kind: ContainerRuntimeConfig | ||
spec: | ||
containerRuntimeConfig: | ||
defaultRuntime: crun | ||
expected: | | ||
apiVersion: machineconfiguration.openshift.io/v1 | ||
kind: ContainerRuntimeConfig | ||
spec: | ||
containerRuntimeConfig: | ||
defaultRuntime: crun | ||
- name: Should be able to set runc in ContainerRuntimeConfig with other params | ||
initial: | | ||
apiVersion: machineconfiguration.openshift.io/v1 | ||
kind: ContainerRuntimeConfig | ||
spec: | ||
containerRuntimeConfig: | ||
defaultRuntime: runc | ||
logLevel: info | ||
expected: | | ||
apiVersion: machineconfiguration.openshift.io/v1 | ||
kind: ContainerRuntimeConfig | ||
spec: | ||
containerRuntimeConfig: | ||
defaultRuntime: runc | ||
logLevel: info | ||
onUpdate: | ||
- name: Case 1 - Change another field - Should be able to update other parameters with invalid defaultRuntime | ||
initialCRDPatches: | ||
- op: remove | ||
path: /spec/versions/0/schema/openAPIV3Schema/properties/spec/properties/containerRuntimeConfig/properties/defaultRuntime/enum | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why remove the enum on thius one, "" is a valid choice no? |
||
initial: | | ||
apiVersion: machineconfiguration.openshift.io/v1 | ||
kind: ContainerRuntimeConfig | ||
spec: | ||
containerRuntimeConfig: | ||
defaultRuntime: docker | ||
logLevel: fatal | ||
updated: | | ||
apiVersion: machineconfiguration.openshift.io/v1 | ||
kind: ContainerRuntimeConfig | ||
spec: | ||
containerRuntimeConfig: | ||
defaultRuntime: docker | ||
logLevel: info | ||
expected: | | ||
apiVersion: machineconfiguration.openshift.io/v1 | ||
kind: ContainerRuntimeConfig | ||
spec: | ||
containerRuntimeConfig: | ||
defaultRuntime: docker | ||
logLevel: info | ||
- name: Case 2 - Remove the field - Should be able to update from invalid to removed | ||
initialCRDPatches: | ||
- op: remove | ||
path: /spec/versions/0/schema/openAPIV3Schema/properties/spec/properties/containerRuntimeConfig/properties/defaultRuntime/enum | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why remove the enum on this one? runc is a valid choice no? |
||
initial: | | ||
apiVersion: machineconfiguration.openshift.io/v1 | ||
kind: ContainerRuntimeConfig | ||
spec: | ||
containerRuntimeConfig: | ||
defaultRuntime: docker | ||
updated: | | ||
apiVersion: machineconfiguration.openshift.io/v1 | ||
kind: ContainerRuntimeConfig | ||
spec: | ||
containerRuntimeConfig: {} | ||
expected: | | ||
apiVersion: machineconfiguration.openshift.io/v1 | ||
kind: ContainerRuntimeConfig | ||
spec: | ||
containerRuntimeConfig: {} | ||
- name: Case 3 - Update the field - Should be able to update from invalid to correct value | ||
initialCRDPatches: | ||
- op: remove | ||
path: /spec/versions/0/schema/openAPIV3Schema/properties/spec/properties/containerRuntimeConfig/properties/defaultRuntime/enum | ||
initial: | | ||
apiVersion: machineconfiguration.openshift.io/v1 | ||
kind: ContainerRuntimeConfig | ||
spec: | ||
containerRuntimeConfig: | ||
defaultRuntime: docker | ||
updated: | | ||
apiVersion: machineconfiguration.openshift.io/v1 | ||
kind: ContainerRuntimeConfig | ||
spec: | ||
containerRuntimeConfig: | ||
defaultRuntime: runc | ||
expected: | | ||
apiVersion: machineconfiguration.openshift.io/v1 | ||
kind: ContainerRuntimeConfig | ||
spec: | ||
containerRuntimeConfig: | ||
defaultRuntime: runc |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.