Move CLI bootstrap into internal package#306
Merged
Conversation
Reviewer's GuideRefactors the CLI startup flow by moving all bootstrapping and flag-handling logic from the public cmd/lets entrypoint into an internal cli package with a reusable Main(version, buildDate) function, leaving main.go as a thin shim, and documents the change in the changelog. Class diagram for internal cli package structureclassDiagram
class cli {
+int Main(version string, buildDate string)
-context.Context getContext()
-int getExitCode(err error, defaultCode int)
-bool failOnConfigError(root *cobra.Command, current *cobra.Command, rootFlags *flags)
-*flags parseRootFlags(args []string) error
}
class flags {
+string config
+int debug
+bool help
+bool version
+bool all
+bool init
+bool upgrade
}
cli --> flags : uses
cli ..> cobra_Command : uses
cli ..> executor_DependencyError : inspects
cli ..> cmd : uses
cli ..> config : uses
cli ..> env : uses
cli ..> logging : uses
cli ..> set : uses
cli ..> upgrade : uses
cli ..> registry : uses
cli ..> workdir : uses
class cobra_Command {
+InitDefaultHelpFlag()
+InitDefaultVersionFlag()
+InitDefaultHelpCmd()
+Traverse(args []string)
+ExecuteContext(ctx context.Context) error
+Flags() *pflagFlagSet
+Name() string
}
class executor_DependencyError {
+error Error
+ExitCode() int
}
class pflagFlagSet {
+int NFlag()
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Now that the startup logic lives in internal/cli, consider decoupling it from process globals (os.Args, os.Stdout/Stderr, environment) by passing these in as parameters to Main (or a lower-level helper) to make the CLI flow easier to test and reuse.
- Main currently hardcodes reading from os.Args again; since you already have the traversed args from rootCmd.Traverse, it may be cleaner to pass those into any helpers that need them rather than reaching back to os.Args for checks like
len(os.Args) == 1.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that the startup logic lives in internal/cli, consider decoupling it from process globals (os.Args, os.Stdout/Stderr, environment) by passing these in as parameters to Main (or a lower-level helper) to make the CLI flow easier to test and reuse.
- Main currently hardcodes reading from os.Args again; since you already have the traversed args from rootCmd.Traverse, it may be cleaner to pass those into any helpers that need them rather than reaching back to os.Args for checks like `len(os.Args) == 1`.
## Individual Comments
### Comment 1
<location path="internal/cli/cli.go" line_range="190-199" />
<code_context>
-// cobra will parse all --config flags, but take only latest
-//
-// --config=myconfig, and this is wrong.
-func parseRootFlags(args []string) (*flags, error) {
- f := &flags{}
- // if first arg is not a flag, then it is subcommand
- if len(args) > 0 && !strings.HasPrefix(args[0], "-") {
- return f, nil
- }
-
- visited := set.NewSet[string]()
-
- isFlagVisited := func(name string) bool {
- if visited.Contains(name) {
- return true
- }
-
- visited.Add(name)
-
- return false
- }
-
- idx := 0
- for idx < len(args) {
- arg := args[idx]
- if !strings.HasPrefix(arg, "-") {
- // stop if arg is not a flag, it is probably a subcommand
- break
- }
-
- name, value, found := strings.Cut(arg, "=")
- switch name {
- case "--config", "-c":
- if !isFlagVisited("config") {
- if found {
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against out-of-bounds access when parsing --config without an inline value.
When `--config` is the last argument (e.g. `lets --config`), `len(args[idx:]) > 0` can be true while `args[idx+1]` is out of range, causing a panic. Tighten the check to ensure there is a next argument, e.g. `len(args) > idx+1` or `len(args[idx:]) > 1`, so you can return a proper error instead of panicking.
</issue_to_address>
### Comment 2
<location path="internal/cli/cli.go" line_range="134" />
<code_context>
- }
-}
-
-// getContext returns context and kicks of a goroutine
-// which waits for SIGINT, SIGTERM and cancels global context.
-//
</code_context>
<issue_to_address>
**nitpick (typo):** Minor typo in comment: "kicks of" should be "kicks off".
In the comment above `getContext`, please change "kicks of a goroutine" to "kicks off a goroutine".
```suggestion
// getContext returns context and kicks off a goroutine
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+190
to
+199
| func parseRootFlags(args []string) (*flags, error) { | ||
| f := &flags{} | ||
| // if first arg is not a flag, then it is subcommand | ||
| if len(args) > 0 && !strings.HasPrefix(args[0], "-") { | ||
| return f, nil | ||
| } | ||
|
|
||
| visited := set.NewSet[string]() | ||
|
|
||
| isFlagVisited := func(name string) bool { |
There was a problem hiding this comment.
issue (bug_risk): Guard against out-of-bounds access when parsing --config without an inline value.
When --config is the last argument (e.g. lets --config), len(args[idx:]) > 0 can be true while args[idx+1] is out of range, causing a panic. Tighten the check to ensure there is a next argument, e.g. len(args) > idx+1 or len(args[idx:]) > 1, so you can return a proper error instead of panicking.
| return 0 | ||
| } | ||
|
|
||
| // getContext returns context and kicks of a goroutine |
There was a problem hiding this comment.
nitpick (typo): Minor typo in comment: "kicks of" should be "kicks off".
In the comment above getContext, please change "kicks of a goroutine" to "kicks off a goroutine".
Suggested change
| // getContext returns context and kicks of a goroutine | |
| // getContext returns context and kicks off a goroutine |
Contributor
Greptile SummaryThis PR is a mechanical refactoring that moves the Key changes:
Review notes:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant M as cmd/lets/main.go
participant C as internal/cli/cli.go
participant R as internal/cmd (rootCmd)
participant CF as internal/config
participant E as internal/executor
M->>C: cli.Main(Version, BuildDate)
C->>C: getContext() — starts signal goroutine
C->>R: CreateRootCommand(version, buildDate)
C->>R: Traverse(os.Args[1:])
C->>C: parseRootFlags(args)
alt --version flag
C->>R: PrintVersionMessage()
C-->>M: return 0
else --init flag
C->>CF: workdir.InitLetsFile()
C-->>M: return 0
else --upgrade flag
C->>CF: upgrade.NewBinaryUpgrader().Upgrade()
C-->>M: return 0
else show usage
C->>R: PrintRootHelpMessage()
C-->>M: return 0
else normal execution
C->>CF: config.Load()
C->>R: InitSubCommands()
C->>R: ExecuteContext(ctx)
R->>E: run command
E-->>C: result/error
C-->>M: return exitCode
end
M->>M: os.Exit(exitCode)
Last reviewed commit: "Move main logic into..." |
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
1 participant
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
letsstartup logic intointernal/cli/cli.goand expose aMainhelper that mirrors the gh CLI entry patterncmd/lets/main.goas a thin launcher that simply forwardsVersion/BuildDateto the new helper and exits with its codeTesting
Summary by Sourcery
Move the CLI startup flow into an internal cli package and keep the binary entrypoint as a thin launcher delegating to it.
Enhancements:
Documentation: