Cleanup default runtime in runtime config when setAsDefault=false#1256
Conversation
Pull Request Test Coverage Report for Build 17224664637Details
💛 - Coveralls |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the AddRuntime() implementation for containerd and cri-o to properly handle default runtime configuration when setAsDefault=false. The change ensures that if a runtime being added is currently set as the default runtime AND setAsDefault=false, the default runtime field is cleared from the configuration.
- Added logic to clear default runtime configuration when setAsDefault=false and the runtime matches the current default
- Enhanced test coverage with new test cases for both containerd and cri-o configurations
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/config/engine/crio/crio.go | Added else clause to clear default_runtime when setAsDefault=false and runtime matches current default |
| pkg/config/engine/containerd/config.go | Added else clause to clear default_runtime_name when setAsDefault=false and runtime matches current default |
| pkg/config/engine/crio/crio_test.go | Added test cases for setting runtime as default and clearing default when setAsDefault=false |
| pkg/config/engine/containerd/config_test.go | Added test cases for setting runtime as default and clearing default when setAsDefault=false |
| if defaultRuntime, ok := config.GetPath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "default_runtime_name"}).(string); ok { | ||
| if defaultRuntime == name { | ||
| config.DeletePath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "default_runtime_name"}) | ||
| } | ||
| } |
There was a problem hiding this comment.
What about the following to clean up intendation a bit:
| if defaultRuntime, ok := config.GetPath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "default_runtime_name"}).(string); ok { | |
| if defaultRuntime == name { | |
| config.DeletePath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "default_runtime_name"}) | |
| } | |
| } | |
| if defaultRuntime, ok := config.GetPath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "default_runtime_name"}).(string); ok && defaultRuntime == name { | |
| config.DeletePath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "default_runtime_name"}) | |
| } |
alternatively, it may be more readable if we do:
| if defaultRuntime, ok := config.GetPath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "default_runtime_name"}).(string); ok { | |
| if defaultRuntime == name { | |
| config.DeletePath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "default_runtime_name"}) | |
| } | |
| } | |
| defaultRuntime, ok := config.GetPath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "default_runtime_name"}).(string) | |
| if ok && defaultRuntime == name { | |
| config.DeletePath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "default_runtime_name"}) | |
| } |
which also decreases the nesting but makes the conditional easier to read.
There was a problem hiding this comment.
I prefer the latter suggestion; definitely more readable
There was a problem hiding this comment.
I agree the latter suggestion is more readable. I have updated this now.
| expectedError: nil, | ||
| }, | ||
| { | ||
| description: "runtime already exists in config, not default runtime", |
There was a problem hiding this comment.
I didn't check if it's covered explicilty, but what about the case where default_runtime_name exists but is not set to test?
There was a problem hiding this comment.
Yes, I believe this is covered in a prior test case on line 117 with description: options from default runtime are imported. For this test case, the original config has default_runtime_name: default and the setAsDefault setting is configured as false.
elezar
left a comment
There was a problem hiding this comment.
Thanks @cdesiniotis.
I have proposed a minor cleanup for readability (and a possibly missed test case).
The other question I have is whether we should backport this?
elezar
left a comment
There was a problem hiding this comment.
@cdesiniotis I am giving my approval on this pending my comments being considered / addressed.
This commit updates the AddRuntime() implementation for containerd and cri-o to clear the default runtime field if the runtime we are adding is currently set as the default AND setAsDefault=false. Signed-off-by: Christopher Desiniotis <cdesiniotis@nvidia.com>
da4bb5a to
cab76e2
Compare
This commit updates the AddRuntime() implementation for containerd and cri-o to clear the default runtime field if the runtime we are adding is currently set as the default AND setAsDefault=false.