[no-relnote] Refactor AddRuntime #1279
Conversation
Pull Request Test Coverage Report for Build 17298873827Details
💛 - Coveralls |
This change refactors the AddRuntime function into GetDefaultRuntimeOptions and AddRuntimeWithOptions. This allows for a clearer distinction between SOURCE and DESTINATION configs which should facilitate switching to drop-in files. Signed-off-by: Evan Lezar <elezar@nvidia.com>
aec10b2 to
5d37aba
Compare
| "runtime_type": c.RuntimeType, | ||
| "runtime_root": "", | ||
| "runtime_engine": "", | ||
| "privileged_without_host_devices": false, |
There was a problem hiding this comment.
I wonder if, on this fallback, we should add the SystemdCgroup = true|false based on a function that looks if the system is running systemd.
There was a problem hiding this comment.
The intent of this change is not to add additional behaviour. It is a refactor.
| } | ||
|
|
||
| config = *c.Tree | ||
| // Note: This is deprecated in containerd 1.4.0 and will be removed in 1.5.0 |
There was a problem hiding this comment.
Should we edit the note, now that 1.5.0 is released? and add a warning log.
There was a problem hiding this comment.
The intent of this change is not to add additional behaviour. It is a refactor.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the AddRuntime function across multiple container engine configuration files to improve code organization and clarity. The refactoring separates the extraction of default runtime options from the actual runtime addition logic.
- Refactors
AddRuntimeintoGetDefaultRuntimeOptionsandAddRuntimeWithOptionsmethods - Updates test files to use the new
New()constructor pattern instead of direct struct instantiation - Improves error handling and logging consistency across CRI-O and containerd configurations
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/config/engine/crio/crio_test.go | Updates tests to use New() constructor with functional options |
| pkg/config/engine/crio/crio.go | Refactors AddRuntime into separate methods for options extraction and runtime addition |
| pkg/config/engine/containerd/config_v1.go | Adapts containerd v1 config to use new refactored method structure |
| pkg/config/engine/containerd/config.go | Implements the new method structure for containerd config with default options fallback |
| defaultRuntimeOptions := c.GetDefaultRuntimeOptions() | ||
| return c.AddRuntimeWithOptions(name, path, setAsDefault, defaultRuntimeOptions) |
There was a problem hiding this comment.
[nitpick] The variable name defaultRuntimeOptions could be shortened to options for better readability, as the context already makes it clear these are default options.
| defaultRuntimeOptions := c.GetDefaultRuntimeOptions() | |
| return c.AddRuntimeWithOptions(name, path, setAsDefault, defaultRuntimeOptions) | |
| options := c.GetDefaultRuntimeOptions() | |
| return c.AddRuntimeWithOptions(name, path, setAsDefault, options) |
Copilot uses AI. Check for mistakes.
| break | ||
| options := c.GetSubtreeByPath([]string{"crio", "runtime", "runtimes", r}) | ||
| if options != nil { | ||
| c.Logger.Debugf("Using options from runtime %v: %v", r, options) |
There was a problem hiding this comment.
[nitpick] The log message capitalization should be consistent with other log messages in the codebase. Consider using lowercase 'using' to match the pattern in the original code.
| c.Logger.Debugf("Using options from runtime %v: %v", r, options) | |
| c.Logger.Debugf("using options from runtime %v: %v", r, options) |
Copilot uses AI. Check for mistakes.
| } | ||
| c.Logger.Warningf("Could not infer options from runtimes %v", runtimeNamesForConfig) |
There was a problem hiding this comment.
[nitpick] The log message capitalization should be consistent. Consider using lowercase 'could' to match the pattern in the original code which used 'could not infer options'.
| c.Logger.Warningf("Could not infer options from runtimes %v", runtimeNamesForConfig) | |
| c.Logger.Warningf("could not infer options from runtimes %v", runtimeNamesForConfig) |
Copilot uses AI. Check for mistakes.
This change refactors the AddRuntime function into GetDefaultRuntimeOptions and AddRuntimeWithOptions. This allows for a clearer distinction between SOURCE and DESTINATION configs which should facilitate switching to drop-in files.