Add EnvVars option for all nvidia-ctk cdi commands #1123
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR introduces environment variable support for various nvidia-ctk cdi CLI flags, allowing defaults to be set via NVIDIA_CTK_CDI_* variables.
- Adds
EnvVarsdefinitions to flags in thetransform root,list, andgeneratecommands. - Ensures each CLI option can now read from the corresponding
NVIDIA_CTK_CDI_*environment variable.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| cmd/nvidia-ctk/cdi/transform/root/root.go | Added EnvVars to all flags of the transform root subcommand |
| cmd/nvidia-ctk/cdi/list/list.go | Added EnvVars to the list command’s spec-dir flag |
| cmd/nvidia-ctk/cdi/generate/generate.go | Added EnvVars to multiple flags in the generate subcommand |
Comments suppressed due to low confidence (4)
cmd/nvidia-ctk/cdi/transform/root/root.go:76
- No tests have been added to verify that the new
EnvVarsfields actually populate the CLI flags from environment variables. Consider adding unit or integration tests to cover this behavior.
EnvVars: []string{"NVIDIA_CTK_CDI_TRANSFORM_ROOT_FROM"},
cmd/nvidia-ctk/cdi/list/list.go:67
- The addition of the
EnvVarshere isn’t accompanied by tests to confirm thatNVIDIA_CTK_CDI_LIST_SPEC_DIRis correctly read. Please add tests to validate this new env var behavior.
EnvVars: []string{"NVIDIA_CTK_CDI_LIST_SPEC_DIR"},
cmd/nvidia-ctk/cdi/generate/generate.go:99
- There are several new
EnvVarsentries in this file but no corresponding tests. Add coverage to ensure eachNVIDIA_CTK_CDI_GENERATE_*variable is applied correctly.
EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_CONFIG_SEARCH_PATH"},
cmd/nvidia-ctk/cdi/transform/root/root.go:75
- Consider updating the flag
Usagestrings to mention the corresponding environment variables (e.g., “can also be set viaNVIDIA_CTK_CDI_TRANSFORM_ROOT_FROM”) so users are aware of the new option.
Usage: "specify the root to be transformed",
This was referenced Jun 2, 2025
elezar
reviewed
Jun 2, 2025
| }, | ||
| &cli.StringFlag{ | ||
| Name: "output", | ||
| Usage: "Specify the file to output the generated CDI specification to. If this is '' the specification is output to STDOUT", | ||
| Destination: &opts.output, | ||
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_OUTPUT"}, |
There was a problem hiding this comment.
Would NVIDIA_CTK_OUTPUT or NVIDIA_CTK_CDI_OUTPUT make sense here? Do we want to be more verbose and have NVIDIA_CTK_CDI_OUTPUT_FILENAME?
There was a problem hiding this comment.
Specify the file to output
Specify the output file path. And that makes me want an env var name ending on _PATH
:)
There was a problem hiding this comment.
NVIDIA_CTK_CDI_OUTPUT_FILE_PATH
elezar
reviewed
Jun 2, 2025
| }, | ||
| &cli.StringFlag{ | ||
| Name: "format", | ||
| Usage: "The output format for the generated spec [json | yaml]. This overrides the format defined by the output file extension (if specified).", | ||
| Value: spec.FormatYAML, | ||
| Destination: &opts.format, | ||
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_FORMAT"}, |
There was a problem hiding this comment.
Suggested change
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_FORMAT"}, | |
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_OUTPUT_FORMAT"}, |
elezar
reviewed
Jun 2, 2025
| }, | ||
| &cli.StringFlag{ | ||
| Name: "dev-root", | ||
| Usage: "Specify the root where `/dev` is located. If this is not specified, the driver-root is assumed.", | ||
| Destination: &opts.devRoot, | ||
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_DEV_ROOT"}, |
There was a problem hiding this comment.
I think NVIDIA_CTK_DEV_ROOT makes sense here. This is the kind of option that should be consistent across the entire CLI.
elezar
reviewed
Jun 2, 2025
| }, | ||
| &cli.StringSliceFlag{ | ||
| Name: "library-search-path", | ||
| Usage: "Specify the path to search for libraries when discovering the entities that should be included in the CDI specification.\n\tNote: This option only applies to CSV mode.", | ||
| Destination: &opts.librarySearchPaths, | ||
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_LIBRARY_SEARCH_PATH"}, |
There was a problem hiding this comment.
Suggested change
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_LIBRARY_SEARCH_PATH"}, | |
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_LIBRARY_SEARCH_PATHS"}, |
elezar
reviewed
Jun 2, 2025
| @@ -145,36 +153,42 @@ func (m command) build() *cli.Command { | |||
| "If not specified, the PATH will be searched for `nvidia-cdi-hook`. " + | |||
| "NOTE: That if this is specified as `nvidia-ctk`, the PATH will be searched for `nvidia-ctk` instead.", | |||
| Destination: &opts.nvidiaCDIHookPath, | |||
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_NVIDIA_CDI_HOOK_PATH"}, | |||
There was a problem hiding this comment.
Suggested change
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_NVIDIA_CDI_HOOK_PATH"}, | |
| EnvVars: []string{"NVIDIA_CTK_CDI_HOOK_PATH"}, |
or
Suggested change
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_NVIDIA_CDI_HOOK_PATH"}, | |
| EnvVars: []string{"NVIDIA_CDI_HOOK_PATH"}, |
since I would also expect this to be globally consistent.
elezar
reviewed
Jun 2, 2025
| }, | ||
| &cli.StringSliceFlag{ | ||
| Name: "csv.file", | ||
| Usage: "The path to the list of CSV files to use when generating the CDI specification in CSV mode.", | ||
| Value: cli.NewStringSlice(csv.DefaultFileList()...), | ||
| Destination: &opts.csv.files, | ||
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_CSV_FILE"}, |
There was a problem hiding this comment.
Suggested change
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_CSV_FILE"}, | |
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_CSV_FILES"}, |
elezar
reviewed
Jun 2, 2025
| }, | ||
| &cli.StringSliceFlag{ | ||
| Name: "device-name-strategy", | ||
| Usage: "Specify the strategy for generating device names. If this is specified multiple times, the devices will be duplicated for each strategy. One of [index | uuid | type-index]", | ||
| Value: cli.NewStringSlice(nvcdi.DeviceNameStrategyIndex, nvcdi.DeviceNameStrategyUUID), | ||
| Destination: &opts.deviceNameStrategies, | ||
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_DEVICE_NAME_STRATEGY"}, |
There was a problem hiding this comment.
Suggested change
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_DEVICE_NAME_STRATEGY"}, | |
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_DEVICE_NAME_STRATEGIES"}, |
elezar
reviewed
Jun 2, 2025
| }, | ||
| &cli.StringFlag{ | ||
| Name: "driver-root", | ||
| Usage: "Specify the NVIDIA GPU driver root to use when discovering the entities that should be included in the CDI specification.", | ||
| Destination: &opts.driverRoot, | ||
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_DRIVER_ROOT"}, |
There was a problem hiding this comment.
Suggested change
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_DRIVER_ROOT"}, | |
| EnvVars: []string{"NVIDIA_CTK_DRIVER_ROOT"}, |
elezar
reviewed
Jun 2, 2025
There was a problem hiding this comment.
Let's leave the transform command out of scope for this PR.
elezar
reviewed
Jun 2, 2025
cmd/nvidia-ctk/cdi/list/list.go
Outdated
| @@ -64,6 +64,7 @@ func (m command) build() *cli.Command { | |||
| Usage: "specify the directories to scan for CDI specifications", | |||
| Value: cli.NewStringSlice(cdi.DefaultSpecDirs...), | |||
| Destination: &cfg.cdiSpecDirs, | |||
| EnvVars: []string{"NVIDIA_CTK_CDI_LIST_SPEC_DIR"}, | |||
There was a problem hiding this comment.
How about:
Suggested change
| EnvVars: []string{"NVIDIA_CTK_CDI_LIST_SPEC_DIR"}, | |
| EnvVars: []string{"NVIDIA_CTK_CDI_SPEC_DIRS"}, |
Since we would expect them to be globally consistent. Not a blocker though.
elezar
reviewed
Jun 2, 2025
| }, | ||
| &cli.StringSliceFlag{ | ||
| Name: "csv.ignore-pattern", | ||
| Usage: "Specify a pattern the CSV mount specifications.", | ||
| Destination: &opts.csv.ignorePatterns, | ||
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_CSV_IGNORE_PATTERN"}, |
There was a problem hiding this comment.
Suggested change
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_CSV_IGNORE_PATTERN"}, | |
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_CSV_IGNORE_PATTERNS"}, |
elezar
reviewed
Jun 2, 2025
| @@ -96,17 +96,20 @@ func (m command) build() *cli.Command { | |||
| Name: "config-search-path", | |||
| Usage: "Specify the path to search for config files when discovering the entities that should be included in the CDI specification.", | |||
| Destination: &opts.configSearchPaths, | |||
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_CONFIG_SEARCH_PATH"}, | |||
There was a problem hiding this comment.
Suggested change
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_CONFIG_SEARCH_PATH"}, | |
| EnvVars: []string{"NVIDIA_CTK_CDI_GENERATE_CONFIG_SEARCH_PATHS"}, |
ArangoGutierrez
force-pushed
the
cdi_generate_env_cli
branch
from
01077b4 to
e33da24
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces support for setting default values of various nvidia-ctk cdi CLI flags via environment variables.
- Adds an
EnvVarsentry for thelistcommand’sspec-dirsflag. - Adds
EnvVarsentries for multiple flags in thegeneratecommand.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| cmd/nvidia-ctk/cdi/list/list.go | Added EnvVars for the spec-dirs flag. |
| cmd/nvidia-ctk/cdi/generate/generate.go | Added EnvVars for several generate command flags. |
Comments suppressed due to low confidence (3)
cmd/nvidia-ctk/cdi/generate/generate.go:128
- [nitpick] The environment variable name here omits the
CDI_GENERATEsegment—consider renaming toNVIDIA_CTK_CDI_GENERATE_DEV_ROOTfor consistency with other flags.
EnvVars: []string{"NVIDIA_CTK_DEV_ROOT"},
cmd/nvidia-ctk/cdi/generate/generate.go:141
- [nitpick] The environment variable name here omits the
CDI_GENERATEsegment—consider renaming toNVIDIA_CTK_CDI_GENERATE_DRIVER_ROOTto align with the pattern used by other flags in this command.
EnvVars: []string{"NVIDIA_CTK_DRIVER_ROOT"},
cmd/nvidia-ctk/cdi/generate/generate.go:156
- [nitpick] The environment variable name here omits
GENERATE—consider renaming toNVIDIA_CTK_CDI_GENERATE_HOOK_PATHfor consistency with other generate command flags.
EnvVars: []string{"NVIDIA_CTK_CDI_HOOK_PATH"},
1e2d93b to
d44a199
Compare
Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
ArangoGutierrez
force-pushed
the
cdi_generate_env_cli
branch
from
d44a199 to
ce3e2c1
Compare
Pull Request Test Coverage Report for Build 15463397348Details
💛 - Coveralls |
elezar
approved these changes
Jun 5, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request introduces environment variable support for various CLI flags across multiple commands in the
nvidia-ctk cdicommand.These changes enhance configurability by allowing users to set default values for CLI flags via environment variables.
ENV VAR follows a logic of
NVIDIA_CTK_CDI_*