-
Notifications
You must be signed in to change notification settings - Fork 71
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
feature(CLI): CLI overhaul improvements #2523
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I watched both the Loom videos. Sounded good if the rest of the team is onboard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new structure is pretty interesting, and I believe we can proceed with it.
I've added some comments on things we could consider in the future, but they can be addressed in other PRs.
Good job @xoscar !
cli/flows/defatults.go
Outdated
@@ -0,0 +1,39 @@ | |||
package flows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit: the file name should be defaults
instead of defatults
, right?
cli/flows/server.go
Outdated
|
||
func GetServer() Server { | ||
if server.Cmd == nil { | ||
panic("server not initialized") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can be more specific here to avoid confusion with the Tracetest API Server start:
panic("server not initialized") | |
panic("server command was not initialized") |
|
||
func WithTeardownMiddleware(setup *global_setup.Setup) Middleware { | ||
return func(runFn RunFn) RunFn { | ||
return func(cmd *cobra.Command, args []string) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This command feels sometimes shady for me because it obliges us to know that we need to do a teardown. What happens if do we forget to call it and use setup.Logger
or analytics
?
To think later: is there a way to keep these commands register to suffer the cleanup only if used? With that we could keep the golang behavior:
resource := someAction()
defer resourceCleanup()
return "", nil | ||
} | ||
|
||
func WithInitAnalytics() SetupOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about the Teardown
discussion: this option feels more to a decorator that uses a resource for the entire command run than just an option. Maybe we could decorate commands that uses analytics and logger to have the startup/teardown logic encapsulated in one place. It makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking on something like this:
func DecorateCommandWithAnalytics(cmd *cobra.Command) cmd {
// decorate PreRun
// decorate PostRun
return decoratedCommand
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I'm going to work on an update on this between hackathon off time haha
cli/resources/setup/setup.go
Outdated
|
||
func WithResourceActions() SetupOption { | ||
return func(setup *Setup, args []string) error { | ||
resourceType := args[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should validate here if there is at least one argument:
resourceType := args[0] | |
if len(args) <= 0 { | |
return fmt.Errorf("you should pass a resourceType to run the setup") | |
} | |
resourceType := args[0] |
) | ||
|
||
func Execute() { | ||
root := global.NewRoot() | ||
global_register.Register(root) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On this command we register all other commands, right? It makes sense to centralize all command registrations in one single file, just to see all commands that the CLI has?
Will close this until I found a better way of handling this |
Reasoning
While working on the CLI improvement tickets there were multiple issues that had to be fixed in order to allow us to scale the codebase, these are:
setupCommand()
Two key issues that showcase the issues we had are:
Where we don't have clear ways or layers to run validations
Command types
Flows. These are interactive commands, guiding users through multiple steps while asking for input
Resources. Everything related to the server resources management
Misc. Simple commands like displaying the version or the dashboard
Global. Space for the root command to be configured
Deprecated. Everything that is no longer active
Encapsulation
Every command type includes the following:
Composing, composing, composing
The global layer contains the base instances and logic for all of the command types, every type can use them as a base to create customized instances depending on what is needed. Example:
The global setup initializes base instances for the config, logger, and analytics
And the resources setup composes a new setup which includes the resource registry and resource actions
Commands can also create a custom setup if required
Middlewares
These are composable functions for the three main command execution sections, pre-run, run, and post-run. Allowing us to share custom logic across the whole command tree.
Similar to the rest of the setup, commands can work without the need of middlewares
New folder structure
Changes
Checklist
Reasoning
https://www.loom.com/share/bfe74cbd8c33443c94275867d42f4bf5
SHOW ME THE MONEY
https://www.loom.com/share/d1d86055da714e2ab11d9d74484b2a62