Skip to content

Conversation

@ArangoGutierrez
Copy link
Collaborator

@ArangoGutierrez ArangoGutierrez commented May 26, 2025

This pull request adds logic for nvidia-ctk cdi generate to load settings from the config.toml file, the order of importance if (1) command line, (2) environment variable(for those flags with an ENV VAR option), (3) config file.

[no-relnote] extra: migrates to urfave v3

Fixes #1075

@ArangoGutierrez ArangoGutierrez requested review from Copilot and elezar May 26, 2025 15:26
@ArangoGutierrez ArangoGutierrez self-assigned this May 26, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request adds logic to load configuration from a config.toml file during CDI generation and listing, ensuring the proper order of precedence when determining settings (command line, environment variable, then config file).

  • Introduces a MockLogger for internal logging tests
  • Extends flag validation in the list and generate commands to incorporate config file values
  • Updates tests to cover configuration loading and flag precedence

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
internal/logger/mock.go Adds a mock logger implementation
cmd/nvidia-ctk/cdi/list/list_test.go Adds tests for flag precedence in the list cmd
cmd/nvidia-ctk/cdi/list/list.go Implements loading config file for list command
cmd/nvidia-ctk/cdi/generate/generate_test.go Adds tests for config loading in generate command
cmd/nvidia-ctk/cdi/generate/generate.go Integrates config file values into generate flags

This comment was marked as outdated.

@ArangoGutierrez
Copy link
Collaborator Author

Let's first agree on #1123 , I'll then rebase this PR and continnue the work here

@ArangoGutierrez ArangoGutierrez added this to the v1.18.0 milestone Jun 4, 2025
@ArangoGutierrez ArangoGutierrez changed the title Load settings from config.toml file during CDI generation #1075 Load settings from config.toml file during CDI generation Jun 4, 2025
@ArangoGutierrez ArangoGutierrez requested a review from Copilot June 4, 2025 16:38

This comment was marked as outdated.

@ArangoGutierrez
Copy link
Collaborator Author

PR updated now with a centralized logic at /internal/config
PTAL @elezar

@coveralls
Copy link

coveralls commented Jun 4, 2025

Pull Request Test Coverage Report for Build 15447921501

Details

  • 107 of 125 (85.6%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.9%) to 34.12%

Changes Missing Coverage Covered Lines Changed/Added Lines %
cmd/nvidia-ctk/cdi/generate/generate.go 12 14 85.71%
cmd/nvidia-ctk/cdi/list/list.go 10 12 83.33%
internal/config/flags.go 85 99 85.86%
Totals Coverage Status
Change from base Build 15437228265: 0.9%
Covered Lines: 4365
Relevant Lines: 12793

💛 - Coveralls

@ArangoGutierrez
Copy link
Collaborator Author

With #763 in I would like this PR to go next
cc @elezar

@ArangoGutierrez ArangoGutierrez force-pushed the i/1075 branch 2 times, most recently from 2453fb2 to 1676931 Compare June 25, 2025 13:46
&cli.StringSliceFlag{
Name: "folder",
Usage: "Specify a folder to add to /etc/ld.so.conf before updating the ld cache",
Destination: &cfg.folders,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a reference for this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moving all the c.something into the c:= cli.Command initialization

This comment was marked as outdated.

Comment on lines 62 to 63
UseShortOptionHandling: true,
EnableShellCompletion: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This functionality is new.

Action: func(ctx context.Context, cmd *cli.Command) error {
return m.run(cmd, &cfg)
Action: func(_ context.Context, _ *cli.Command) error {
return m.run(nil, &cfg)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't pass nil into the run() function. This also doesn't seem to be related to adding a config file.

Copy link
Member

@elezar elezar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last commit has more changes than I would expect -- which I would expect to be limited to the generate.go and up the chain to nvidia-ctk main.go.

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR migrates the CLI to urfave/cli v3, adds a --config flag and config-file support for loading settings (command line, env, config.toml precedence), and updates the generate command to pull default values from a TOML config.

  • Bump Go version and replace urfave/cli/v2 imports with urfave/cli/v3 and add cli-altsrc/v3
  • Introduce configAsValueSource to allow flags to be loaded from config.toml in cdi generate
  • Update all subcommands to use Sources: cli.EnvVars and the new context-based Before/Action signatures

Reviewed Changes

Copilot reviewed 35 out of 159 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
go.mod Updated Go version, added toolchain, switched CLI deps
cmd/nvidia-ctk/main.go Converted root CLI to v3, added global --config flag
cmd/nvidia-ctk/config/create-default/create-default.go Migrated to v3 but removed validation call
cmd/nvidia-ctk/cdi/generate/config.go Added configAsValueSource for TOML-backed flag sources
cmd/nvidia-ctk/system/system.go Added unused configFile field in system commands
many others Updated Before/Action signatures, replaced EnvVars
Comments suppressed due to low confidence (2)

cmd/nvidia-ctk/cdi/generate/config.go:31

  • [nitpick] Consider adding unit tests for configAsValueSource (e.g., From, loadIfRequired, ValueSource.Lookup) to verify configuration flags are correctly loaded from TOML files.
func (o *options) Config() *configAsValueSource {

cmd/nvidia-ctk/config/create-default/create-default.go:71

  • This validateFlags implementation always returns nil, disabling the previous opts.Validate() logic. Restore a call to opts.Validate() or add equivalent checks to ensure flags are valid.
func (m command) validateFlags(c *cli.Command, opts *flags.Options) error {

ArangoGutierrez and others added 5 commits July 8, 2025 12:08
Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
Signed-off-by: Evan Lezar <[email protected]>
@elezar elezar force-pushed the i/1075 branch 2 times, most recently from ca6cc0f to e31290a Compare July 8, 2025 11:51
Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
Copy link
Member

@elezar elezar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the migration to v3 will be valuable for some of our other projects too -- although this is not critical.

@ArangoGutierrez ArangoGutierrez merged commit 35babbe into NVIDIA:main Jul 8, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Load settings from config.toml file during CDI generation

3 participants