Skip to content
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

Flatten cmd/ constructors for sub-commands #2417

Closed
na-- opened this issue Mar 8, 2022 · 1 comment · Fixed by #2418
Closed

Flatten cmd/ constructors for sub-commands #2417

na-- opened this issue Mar 8, 2022 · 1 comment · Fixed by #2418
Labels
Milestone

Comments

@na--
Copy link
Member

na-- commented Mar 8, 2022

Now that #2410 and #2412 introduce a unified way of constructing *cobra.Command sub-commands:

k6/cmd/root.go

Lines 242 to 246 in e4241ca

rootCmd.AddCommand(
getArchiveCmd(gs), getCloudCmd(gs), getConvertCmd(gs), getInspectCmd(gs),
loginCmd, getPauseCmd(gs), getResumeCmd(gs), getScaleCmd(gs), getRunCmd(gs),
getStatsCmd(gs), getStatusCmd(gs), getVersionCmd(gs),
)

Instead of having functions that are hundreds of lines of code and starting indented 3 levels deep:

k6/cmd/run.go

Lines 80 to 83 in e4241ca

RunE: func(cmd *cobra.Command, args []string) error {
printBanner(globalState)
test, err := loadTest(globalState, cmd, args, getConfig)

We can introduce custom helper types for some of the bigger sub-commands (e.g. k6 run, k6 cloud, etc.) that split apart these monstrosities and flatten then 🎉 Something like this:

func getCmdRun(gs *globalState) *cobra.Command {
    c := &cmdRun{gs}

    cmd := &cobra.Command{
        Use:   "run",
        // ...
        RunE: c.run,
    }
    cmd.Flags().SortFlags = false
    cmd.Flags().AddFlagSet(c.flagSet())

    return cmd
}

type cmdRun struct {
    gs *globalState
}

func (c *cmdRun) run(cmd *cobra.Command, args []string) error {
    printBanner(c.gs)

    test, err := loadTest(c.gs, cmd, args, getPartialConfig)
    if err != nil {
        return err
    }

    // ...
    // rest of the code, potentially split apart in multiple cmdRun methods
    // ...
}

func (c *cmdRun) flagSet() *pflag.FlagSet {
    // ...
}

Tons of benefits:

  • no unnecessary indentation
  • can split apart huge functions nicely
  • every sub-command struct can hold state, if needs be
  • it can also have properties for the per-command flags like these, so they are not loose variables in a quasi-global scope:
    archiveOut := "archive.tar"

    k6/cmd/cloud.go

    Lines 49 to 50 in e4241ca

    showCloudLogs := true
    exitOnRunning := false
  • loose helper functions like these, that are specific for a single command, can become methods:
    func inspectOutputWithExecRequirements(gs *globalState, cmd *cobra.Command, test *loadedTest) (interface{}, error) {

    k6/cmd/cloud.go

    Line 330 in e4241ca

    func cloudCmdFlagSet(showCloudLogs, exitOnRunning *bool) *pflag.FlagSet {

    k6/cmd/run.go

    Line 341 in e4241ca

    func handleSummaryResult(fs afero.Fs, stdOut, stdErr io.Writer, result map[string]io.Reader) error {
  • this may even decrease the number of instances where we have to pass a bajillion arguments to some constructor function 😅
@na-- na-- added the refactor label Mar 8, 2022
@na--
Copy link
Member Author

na-- commented Mar 8, 2022

Hmm I had a thought that if globalState was exported and moved to another module (it doesn't really need to be in cmd/ anyway 🤷‍♂️), it would be super easy to allow xk6 extensions to add k6 sub-commands 🤔 Given the cobra architecture, it will even be easy to allow them to mess (e.g. add flags) with other sub-commands...

Still not sure if that is a good or a bad idea though 😅 And even if it's a good idea, it should probably wait for the configuration refactoring (#883), so we don't lock ourselves in the current bad state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant