-
Notifications
You must be signed in to change notification settings - Fork 490
refactor: split Execute into Build + Execute with explicit IO and keychain injection #371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
f7e8e35
db422ef
254d7e7
5040edb
bcbc1bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,132 @@ | ||||||||||||||||||
| // Copyright (c) 2026 Lark Technologies Pte. Ltd. | ||||||||||||||||||
| // SPDX-License-Identifier: MIT | ||||||||||||||||||
|
|
||||||||||||||||||
| package cmd | ||||||||||||||||||
|
|
||||||||||||||||||
| import ( | ||||||||||||||||||
| "context" | ||||||||||||||||||
| "io" | ||||||||||||||||||
| "os" | ||||||||||||||||||
|
|
||||||||||||||||||
| "golang.org/x/term" | ||||||||||||||||||
|
|
||||||||||||||||||
| "github.com/larksuite/cli/cmd/api" | ||||||||||||||||||
| "github.com/larksuite/cli/cmd/auth" | ||||||||||||||||||
| "github.com/larksuite/cli/cmd/completion" | ||||||||||||||||||
| cmdconfig "github.com/larksuite/cli/cmd/config" | ||||||||||||||||||
| "github.com/larksuite/cli/cmd/doctor" | ||||||||||||||||||
| "github.com/larksuite/cli/cmd/profile" | ||||||||||||||||||
| "github.com/larksuite/cli/cmd/schema" | ||||||||||||||||||
| "github.com/larksuite/cli/cmd/service" | ||||||||||||||||||
| cmdupdate "github.com/larksuite/cli/cmd/update" | ||||||||||||||||||
| "github.com/larksuite/cli/internal/build" | ||||||||||||||||||
| "github.com/larksuite/cli/internal/cmdutil" | ||||||||||||||||||
| "github.com/larksuite/cli/internal/keychain" | ||||||||||||||||||
| "github.com/larksuite/cli/shortcuts" | ||||||||||||||||||
| "github.com/spf13/cobra" | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
| // BuildOption configures optional aspects of the command tree construction. | ||||||||||||||||||
| type BuildOption func(*buildConfig) | ||||||||||||||||||
|
|
||||||||||||||||||
| type buildConfig struct { | ||||||||||||||||||
| streams *cmdutil.IOStreams | ||||||||||||||||||
| keychain keychain.KeychainAccess | ||||||||||||||||||
| globals GlobalOptions | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // WithIO sets the IO streams for the CLI by wrapping raw reader/writers. | ||||||||||||||||||
| // Terminal detection is derived from the input's underlying *os.File, if any; | ||||||||||||||||||
| // non-file readers (bytes.Buffer, strings.Reader, …) produce IsTerminal=false. | ||||||||||||||||||
| func WithIO(in io.Reader, out, errOut io.Writer) BuildOption { | ||||||||||||||||||
| return func(c *buildConfig) { | ||||||||||||||||||
| isTerminal := false | ||||||||||||||||||
| if f, ok := in.(*os.File); ok { | ||||||||||||||||||
| isTerminal = term.IsTerminal(int(f.Fd())) | ||||||||||||||||||
| } | ||||||||||||||||||
| c.streams = &cmdutil.IOStreams{In: in, Out: out, ErrOut: errOut, IsTerminal: isTerminal} | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // WithKeychain sets the secret storage backend. If not provided, the platform keychain is used. | ||||||||||||||||||
| func WithKeychain(kc keychain.KeychainAccess) BuildOption { | ||||||||||||||||||
| return func(c *buildConfig) { | ||||||||||||||||||
| c.keychain = kc | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // HideProfile sets the visibility policy for the root-level --profile flag. | ||||||||||||||||||
| // When hide is true the flag stays registered (so existing invocations still | ||||||||||||||||||
| // parse) but is omitted from help and shell completion. Typically called as | ||||||||||||||||||
| // HideProfile(isSingleAppMode()). | ||||||||||||||||||
| func HideProfile(hide bool) BuildOption { | ||||||||||||||||||
| return func(c *buildConfig) { | ||||||||||||||||||
| c.globals.HideProfile = hide | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // Build constructs the full command tree without executing. | ||||||||||||||||||
| // Returns only the cobra.Command; Factory is internal. | ||||||||||||||||||
| // Use Execute for the standard production entry point. | ||||||||||||||||||
| func Build(ctx context.Context, inv cmdutil.InvocationContext, opts ...BuildOption) *cobra.Command { | ||||||||||||||||||
| _, rootCmd := buildInternal(ctx, inv, opts...) | ||||||||||||||||||
| return rootCmd | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // buildInternal is a pure assembly function: it wires the command tree from | ||||||||||||||||||
| // inv and BuildOptions alone. Any state-dependent decision (disk, network, | ||||||||||||||||||
| // env) belongs in the caller and must be threaded in via BuildOption. | ||||||||||||||||||
| // | ||||||||||||||||||
| // Callers must supply WithIO; buildInternal intentionally does not default | ||||||||||||||||||
| // the streams so tests and alternative entry points can't silently inherit | ||||||||||||||||||
| // os.Std*. | ||||||||||||||||||
| func buildInternal(ctx context.Context, inv cmdutil.InvocationContext, opts ...BuildOption) (*cmdutil.Factory, *cobra.Command) { | ||||||||||||||||||
| // cfg.globals.Profile is left zero here; it's bound to the --profile | ||||||||||||||||||
| // flag in RegisterGlobalFlags and filled by cobra's parse step. | ||||||||||||||||||
| cfg := &buildConfig{} | ||||||||||||||||||
| for _, o := range opts { | ||||||||||||||||||
| o(cfg) | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+87
to
+89
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Guard against nil functional options to avoid panic. At Line 68, calling Proposed fix for _, o := range opts {
- o(cfg)
+ if o != nil {
+ o(cfg)
+ }
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
|
|
||||||||||||||||||
| f := cmdutil.NewDefault(cfg.streams, inv) | ||||||||||||||||||
| if cfg.keychain != nil { | ||||||||||||||||||
| f.Keychain = cfg.keychain | ||||||||||||||||||
| } | ||||||||||||||||||
| rootCmd := &cobra.Command{ | ||||||||||||||||||
| Use: "lark-cli", | ||||||||||||||||||
| Short: "Lark/Feishu CLI — OAuth authorization, UAT management, API calls", | ||||||||||||||||||
| Long: rootLong, | ||||||||||||||||||
| Version: build.Version, | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| rootCmd.SetContext(ctx) | ||||||||||||||||||
| rootCmd.SetIn(cfg.streams.In) | ||||||||||||||||||
| rootCmd.SetOut(cfg.streams.Out) | ||||||||||||||||||
| rootCmd.SetErr(cfg.streams.ErrOut) | ||||||||||||||||||
|
|
||||||||||||||||||
| installTipsHelpFunc(rootCmd) | ||||||||||||||||||
| rootCmd.SilenceErrors = true | ||||||||||||||||||
|
|
||||||||||||||||||
| RegisterGlobalFlags(rootCmd.PersistentFlags(), &cfg.globals) | ||||||||||||||||||
| rootCmd.PersistentPreRun = func(cmd *cobra.Command, args []string) { | ||||||||||||||||||
| cmd.SilenceUsage = true | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| rootCmd.AddCommand(cmdconfig.NewCmdConfig(f)) | ||||||||||||||||||
| rootCmd.AddCommand(auth.NewCmdAuth(f)) | ||||||||||||||||||
| rootCmd.AddCommand(profile.NewCmdProfile(f)) | ||||||||||||||||||
| rootCmd.AddCommand(doctor.NewCmdDoctor(f)) | ||||||||||||||||||
| rootCmd.AddCommand(api.NewCmdApiWithContext(ctx, f, nil)) | ||||||||||||||||||
| rootCmd.AddCommand(schema.NewCmdSchema(f, nil)) | ||||||||||||||||||
| rootCmd.AddCommand(completion.NewCmdCompletion(f)) | ||||||||||||||||||
| rootCmd.AddCommand(cmdupdate.NewCmdUpdate(f)) | ||||||||||||||||||
| service.RegisterServiceCommandsWithContext(ctx, rootCmd, f) | ||||||||||||||||||
| shortcuts.RegisterShortcutsWithContext(ctx, rootCmd, f) | ||||||||||||||||||
|
|
||||||||||||||||||
| // Prune commands incompatible with strict mode. | ||||||||||||||||||
| if mode := f.ResolveStrictMode(ctx); mode.IsActive() { | ||||||||||||||||||
| pruneForStrictMode(rootCmd, mode) | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| return f, rootCmd | ||||||||||||||||||
| } | ||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| // Copyright (c) 2026 Lark Technologies Pte. Ltd. | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| package cmd | ||
|
|
||
| import ( | ||
| "context" | ||
| "os" | ||
| "testing" | ||
|
|
||
| "github.com/larksuite/cli/internal/cmdutil" | ||
| "github.com/larksuite/cli/internal/core" | ||
| "github.com/spf13/pflag" | ||
| ) | ||
|
|
||
| func testStreams() BuildOption { return WithIO(os.Stdin, os.Stdout, os.Stderr) } | ||
|
|
||
| func TestRegisterGlobalFlags_PolicyVisible(t *testing.T) { | ||
| fs := pflag.NewFlagSet("test", pflag.ContinueOnError) | ||
| opts := &GlobalOptions{} | ||
| RegisterGlobalFlags(fs, opts) | ||
|
|
||
| flag := fs.Lookup("profile") | ||
| if flag == nil { | ||
| t.Fatal("profile flag should be registered") | ||
| } | ||
| if flag.Hidden { | ||
| t.Fatal("profile flag should be visible when HideProfile is false") | ||
| } | ||
| } | ||
|
|
||
| func TestRegisterGlobalFlags_PolicyHidden(t *testing.T) { | ||
| fs := pflag.NewFlagSet("test", pflag.ContinueOnError) | ||
| opts := &GlobalOptions{HideProfile: true} | ||
| RegisterGlobalFlags(fs, opts) | ||
|
|
||
| flag := fs.Lookup("profile") | ||
| if flag == nil { | ||
| t.Fatal("profile flag should be registered") | ||
| } | ||
| if !flag.Hidden { | ||
| t.Fatal("profile flag should be hidden when HideProfile is true") | ||
| } | ||
| if err := fs.Parse([]string{"--profile", "x"}); err != nil { | ||
| t.Fatalf("Parse() error = %v; hidden flag should still parse", err) | ||
| } | ||
| if opts.Profile != "x" { | ||
| t.Fatalf("opts.Profile = %q, want %q", opts.Profile, "x") | ||
| } | ||
| } | ||
|
|
||
| func TestIsSingleAppMode_NoConfig(t *testing.T) { | ||
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) | ||
| if !isSingleAppMode() { | ||
| t.Fatal("isSingleAppMode() = false, want true when no config exists") | ||
| } | ||
| } | ||
|
|
||
| func TestIsSingleAppMode_SingleApp(t *testing.T) { | ||
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) | ||
| saveAppsForTest(t, []core.AppConfig{ | ||
| {Name: "default", AppId: "cli_a", AppSecret: core.PlainSecret("x"), Brand: core.BrandFeishu}, | ||
| }) | ||
| if !isSingleAppMode() { | ||
| t.Fatal("isSingleAppMode() = false, want true for single-app config") | ||
| } | ||
| } | ||
|
|
||
| func TestIsSingleAppMode_MultiApp(t *testing.T) { | ||
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) | ||
| saveAppsForTest(t, []core.AppConfig{ | ||
| {Name: "a", AppId: "cli_a", AppSecret: core.PlainSecret("x"), Brand: core.BrandFeishu}, | ||
| {Name: "b", AppId: "cli_b", AppSecret: core.PlainSecret("y"), Brand: core.BrandFeishu}, | ||
| }) | ||
| if isSingleAppMode() { | ||
| t.Fatal("isSingleAppMode() = true, want false for multi-app config") | ||
| } | ||
| } | ||
|
|
||
| func TestBuildInternal_HideProfileOption(t *testing.T) { | ||
| _, root := buildInternal(context.Background(), cmdutil.InvocationContext{}, testStreams(), HideProfile(true)) | ||
|
|
||
| flag := root.PersistentFlags().Lookup("profile") | ||
| if flag == nil { | ||
| t.Fatal("profile flag should be registered") | ||
| } | ||
| if !flag.Hidden { | ||
| t.Fatal("profile flag should be hidden when HideProfile(true) is applied") | ||
| } | ||
| } | ||
|
|
||
| func TestBuildInternal_DefaultShowsProfileFlag(t *testing.T) { | ||
| _, root := buildInternal(context.Background(), cmdutil.InvocationContext{}, testStreams()) | ||
|
|
||
| flag := root.PersistentFlags().Lookup("profile") | ||
| if flag == nil { | ||
| t.Fatal("profile flag should be registered by default") | ||
| } | ||
| if flag.Hidden { | ||
| t.Fatal("profile flag should be visible by default") | ||
| } | ||
| } | ||
|
|
||
| func saveAppsForTest(t *testing.T, apps []core.AppConfig) { | ||
| t.Helper() | ||
| multi := &core.MultiAppConfig{CurrentApp: apps[0].Name, Apps: apps} | ||
| if err := core.SaveMultiAppConfig(multi); err != nil { | ||
| t.Fatalf("SaveMultiAppConfig() error = %v", err) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| // Copyright (c) 2026 Lark Technologies Pte. Ltd. | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| package cmd | ||
|
|
||
| import "github.com/larksuite/cli/internal/vfs" | ||
|
|
||
| // SetDefaultFS replaces the global filesystem implementation used by internal | ||
| // packages. The provided fs must implement the vfs.FS interface. If fs is nil, | ||
| // the default OS filesystem is restored. | ||
| // | ||
| // Call this before Build or Execute to take effect. | ||
| func SetDefaultFS(fs vfs.FS) { | ||
| if fs == nil { | ||
| fs = vfs.OsFs{} | ||
| } | ||
| vfs.DefaultFS = fs | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.