feat: refactor aks-node-controller to use urfave cli to manually do command line parsing, setting#8397
Merged
awesomenix merged 2 commits intomainfrom Apr 24, 2026
Merged
Conversation
…ommand line parsing, setting
awesomenix
requested review from
AbelHu,
Devinwong,
SriHarsha001,
calvin197,
cameronmeissner,
djsly,
ganeshkumarashok,
junjiezhang1997,
lilypan26,
mxj220,
pdamianov-dev,
phealy,
r2k1,
sulixu,
surajssd,
timmy-wright and
zachary-bailey
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors aks-node-controller command-line handling to use urfave/cli/v3, consolidating command wiring into app.go while aiming to preserve existing command behavior and ANC-specific exit-code semantics.
Changes:
- Introduces
urfave/cli/v3as the CLI framework dependency. - Replaces manual
flagparsing + command registry indirection withcli.Commandwiring inapp.go. - Splits operational flows into command-specific helpers (
runProvisionCommand,runProvisionWaitCommand).
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| aks-node-controller/app.go | Replaces manual CLI parsing/dispatch with urfave/cli/v3 commands and new command-specific execution methods |
| aks-node-controller/go.mod | Adds github.com/urfave/cli/v3 dependency (and promotes yaml.v3 to direct) |
| aks-node-controller/go.sum | Adds sums for github.com/urfave/cli/v3 |
Comment on lines
+66
to
+106
| cmd := &cli.Command{ | ||
| Name: "aks-node-controller", | ||
| Usage: "Parse contract and run csecmd", | ||
| Version: Version, | ||
| ExitErrHandler: func(context.Context, *cli.Command, error) { | ||
| // Return errors to the caller so exit codes are derived consistently in one place. | ||
| }, | ||
| Action: func(context.Context, *cli.Command) error { | ||
| if len(args) > 1 { | ||
| return fmt.Errorf("unknown command: %s", args[1]) | ||
| } | ||
| return errors.New("missing command argument") | ||
| }, | ||
| Commands: []*cli.Command{ | ||
| { | ||
| Name: "provision", | ||
| Usage: "Run node provisioning", | ||
| Flags: []cli.Flag{ | ||
| &cli.StringFlag{Name: "provision-config", Usage: "path to the provision config file"}, | ||
| &cli.StringFlag{Name: "nbc-cmd", Usage: "path to the NBC command file"}, | ||
| &cli.BoolFlag{Name: "dry-run", Usage: "print the command that would be run without executing it"}, | ||
| }, | ||
| Action: func(ctx context.Context, cmd *cli.Command) error { | ||
| return a.runProvisionCommand(ctx, ProvisionFlags{ | ||
| ProvisionConfig: cmd.String("provision-config"), | ||
| NBCCmd: cmd.String("nbc-cmd"), | ||
| }, cmd.Bool("dry-run")) | ||
| }, | ||
| }, | ||
| { | ||
| Name: "provision-wait", | ||
| Usage: "Wait for provisioning to complete", | ||
| Action: func(ctx context.Context, cmd *cli.Command) error { | ||
| provisionStatusFiles := ProvisionStatusFiles{ | ||
| ProvisionJSONFile: provisionJSONFilePath, | ||
| ProvisionCompleteFile: provisionCompleteFilePath, | ||
| } | ||
| provisionOutput, err := a.runProvisionWaitCommand(ctx, provisionStatusFiles) | ||
| _, _ = fmt.Fprintln(cmd.Root().Writer, provisionOutput) | ||
| return err | ||
| }, |
There was a problem hiding this comment.
With urfave/cli, flag parsing errors for provision (e.g., unknown flag, missing flag value) will occur before runProvisionCommand runs, so writeCompleteFileOnError(...) is never invoked. That can leave provision.complete/provision.json unwritten and cause provision-wait to block until its context times out. Consider handling CLI parse/usage errors in ExitErrHandler (or equivalent hook) to emit a ProvisionResult (ExitCode=240 + error) and write the sentinel files for provision invocations, matching the previous fail-fast behavior.
awesomenix
force-pushed
the
nishp/fix/e2e/azurecni
branch
from
e7cd152 to
60dcb11
Compare
awesomenix
force-pushed
the
nishp/fix/e2e/azurecni
branch
from
60dcb11 to
85f28e6
Compare
Devinwong
approved these changes
Apr 24, 2026
lilypan26
approved these changes
Apr 24, 2026
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
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.
Summary
Refactor
aks-node-controllerCLI parsing to useurfave/cli/v3and simplify the command wiring so it is much closer to the nativecli.Command{...}pattern.What changed
urfave/cli/v3app.gocmd.goindirectionexecuteCommandwrapper with direct command-specific methods:runProvisionCommand(...)runProvisionWaitCommand(...)provisionprovision-waitversion--help--version--dry-runaks-node-controller startedlog to operational commands only (provision/provision-wait)Why
This change keeps the important ANC-specific behavior (custom exit codes, event logging, self-update on operational paths, fail-fast handling) while making the CLI setup
much easier to read and maintain.
Scope
This PR is intentionally limited to CLI refactoring and simplification.
It does not change the shell/wrapper hotfix installation approach or remove the existing Go self-update flow.