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.

Sign up for GitHub

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

refactor: Create structs for each command to better isolate commands #3322

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+1,527 −1,228
Open

refactor: Create structs for each command to better isolate commands #3322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
179 changes: 100 additions & 79 deletions src/cmd/connect.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,100 +16,121 @@ import (
"github.com/zarf-dev/zarf/src/pkg/utils/exec"
)

var (
// ConnectOptions holds the command-line options for 'connect' sub-command.
type ConnectOptions struct {
cliOnly bool
zt cluster.TunnelInfo
)
var connectCmd = &cobra.Command{
Use: "connect { REGISTRY | GIT | connect-name }",
Aliases: []string{"c"},
Short: lang.CmdConnectShort,
Long: lang.CmdConnectLong,
RunE: func(cmd *cobra.Command, args []string) error {
ctx := cmd.Context()
l := logger.From(ctx)
target := ""
if len(args) > 0 {
target = args[0]
}

spinner := message.NewProgressSpinner(lang.CmdConnectPreparingTunnel, target)
defer spinner.Stop()
}

c, err := cluster.NewCluster()
if err != nil {
return err
}
// NewConnectCommand creates the `connect` sub-command and its nested children.
func NewConnectCommand() *cobra.Command {
o := &ConnectOptions{}

var tunnel *cluster.Tunnel
if target == "" {
tunnel, err = c.ConnectTunnelInfo(ctx, zt)
} else {
var ti cluster.TunnelInfo
ti, err = c.NewTargetTunnelInfo(ctx, target)
if err != nil {
return fmt.Errorf("unable to create tunnel: %w", err)
}
if zt.LocalPort != 0 {
ti.LocalPort = zt.LocalPort
}
tunnel, err = c.ConnectTunnelInfo(ctx, ti)
}
cmd := &cobra.Command{
Use: "connect { REGISTRY | GIT | connect-name }",
Aliases: []string{"c"},
Short: lang.CmdConnectShort,
Long: lang.CmdConnectLong,
RunE: o.Run,
}

if err != nil {
return fmt.Errorf("unable to connect to the service: %w", err)
}
cmd.Flags().StringVar(&o.zt.ResourceName, "name", "", lang.CmdConnectFlagName)
cmd.Flags().StringVar(&o.zt.Namespace, "namespace", cluster.ZarfNamespaceName, lang.CmdConnectFlagNamespace)
cmd.Flags().StringVar(&o.zt.ResourceType, "type", cluster.SvcResource, lang.CmdConnectFlagType)
cmd.Flags().IntVar(&o.zt.LocalPort, "local-port", 0, lang.CmdConnectFlagLocalPort)
cmd.Flags().IntVar(&o.zt.RemotePort, "remote-port", 0, lang.CmdConnectFlagRemotePort)
cmd.Flags().BoolVar(&o.cliOnly, "cli-only", false, lang.CmdConnectFlagCliOnly)

defer tunnel.Close()

if cliOnly {
spinner.Updatef(lang.CmdConnectEstablishedCLI, tunnel.FullURL())
l.Info("Tunnel established, waiting for user to interrupt (ctrl-c to end)", "url", tunnel.FullURL())
} else {
spinner.Updatef(lang.CmdConnectEstablishedWeb, tunnel.FullURL())
l.Info("Tunnel established, opening your default web browser (ctrl-c to end)", "url", tunnel.FullURL())
if err := exec.LaunchURL(tunnel.FullURL()); err != nil {
return err
}
}
// TODO(soltysh): consider splitting sub-commands into separate files
cmd.AddCommand(NewConnectListCommand())

// Wait for the interrupt signal or an error.
select {
case <-ctx.Done():
spinner.Successf(lang.CmdConnectTunnelClosed, tunnel.FullURL())
return nil
case err = <-tunnel.ErrChan():
return fmt.Errorf("lost connection to the service: %w", err)
}
},
return cmd
}

var connectListCmd = &cobra.Command{
Use: "list",
Aliases: []string{"l"},
Short: lang.CmdConnectListShort,
RunE: func(cmd *cobra.Command, _ []string) error {
c, err := cluster.NewCluster()
// Run performs the execution of 'connect' sub command.
func (o *ConnectOptions) Run(cmd *cobra.Command, args []string) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good choice to isolate these out into function declarations on the pkg

ctx := cmd.Context()
l := logger.From(ctx)
target := ""
if len(args) > 0 {
target = args[0]
}

spinner := message.NewProgressSpinner(lang.CmdConnectPreparingTunnel, target)
defer spinner.Stop()

c, err := cluster.NewCluster()
Copy link
Contributor

@mkcp mkcp Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely not in scope for this PR, but sometime in the future it'd be great to ensure every CLI command can call a clearly defined API fn that encapsulates all of the configuration. I don't think this intent is well captured in an issue atm, but Connect has complex logic that could be abstracted out and made me think of the discussions we've had around it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, definitely. Have a look at https://github.com/kubernetes/kubectl/blob/master/pkg/cmd/events/events.go - that's my ideal command. There's a very strict boundary between the CLI bits (EventsFlags struct) and actual command options (EventsOptions struct).

The flow is you create EventsFlags for the actual command (the goal is to allow easily bundling commands together (wait is a perfect example which is used in delete, and could easily be used elsewhere). ToOptions transforms flags into EventOptions struct, which has two important methods Validate which goal is to validate correctness of all the options fields, and finally Run which executes the code.

I'll be working on followup steps, once we get this in, but due to the amount of changes I want to do it slowly 😅 for ease of reviews.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all tracks - having explicit boundaries and a validation step are great.

if err != nil {
return err
}

var tunnel *cluster.Tunnel
if target == "" {
tunnel, err = c.ConnectTunnelInfo(ctx, o.zt)
} else {
var ti cluster.TunnelInfo
ti, err = c.NewTargetTunnelInfo(ctx, target)
if err != nil {
return err
return fmt.Errorf("unable to create tunnel: %w", err)
}
connections, err := c.ListConnections(cmd.Context())
if err != nil {
if o.zt.LocalPort != 0 {
ti.LocalPort = o.zt.LocalPort
}
tunnel, err = c.ConnectTunnelInfo(ctx, ti)
}

if err != nil {
return fmt.Errorf("unable to connect to the service: %w", err)
}

defer tunnel.Close()

if o.cliOnly {
spinner.Updatef(lang.CmdConnectEstablishedCLI, tunnel.FullURL())
l.Info("Tunnel established, waiting for user to interrupt (ctrl-c to end)", "url", tunnel.FullURL())
} else {
spinner.Updatef(lang.CmdConnectEstablishedWeb, tunnel.FullURL())
l.Info("Tunnel established, opening your default web browser (ctrl-c to end)", "url", tunnel.FullURL())
if err := exec.LaunchURL(tunnel.FullURL()); err != nil {
return err
}
message.PrintConnectStringTable(connections)
}

// Wait for the interrupt signal or an error.
select {
case <-ctx.Done():
spinner.Successf(lang.CmdConnectTunnelClosed, tunnel.FullURL())
return nil
},
case err = <-tunnel.ErrChan():
return fmt.Errorf("lost connection to the service: %w", err)
}
}

func init() {
rootCmd.AddCommand(connectCmd)
connectCmd.AddCommand(connectListCmd)
// ConnectOptions holds the command-line options for 'connect list' sub-command.
type ConnectListOptions struct{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 good pattern


// NewConnectListCommand creates the `connect list` sub-command.
func NewConnectListCommand() *cobra.Command {
o := &ConnectListOptions{}
cmd := &cobra.Command{
Use: "list",
Aliases: []string{"l"},
Short: lang.CmdConnectListShort,
RunE: o.Run,
}
return cmd
}

connectCmd.Flags().StringVar(&zt.ResourceName, "name", "", lang.CmdConnectFlagName)
connectCmd.Flags().StringVar(&zt.Namespace, "namespace", cluster.ZarfNamespaceName, lang.CmdConnectFlagNamespace)
connectCmd.Flags().StringVar(&zt.ResourceType, "type", cluster.SvcResource, lang.CmdConnectFlagType)
connectCmd.Flags().IntVar(&zt.LocalPort, "local-port", 0, lang.CmdConnectFlagLocalPort)
connectCmd.Flags().IntVar(&zt.RemotePort, "remote-port", 0, lang.CmdConnectFlagRemotePort)
connectCmd.Flags().BoolVar(&cliOnly, "cli-only", false, lang.CmdConnectFlagCliOnly)
// Run performs the execution of 'connect list' sub-command.
func (o *ConnectListOptions) Run(cmd *cobra.Command, _ []string) error {
c, err := cluster.NewCluster()
if err != nil {
return err
}
connections, err := c.ListConnections(cmd.Context())
if err != nil {
return err
}
message.PrintConnectStringTable(connections)
return nil
}
159 changes: 83 additions & 76 deletions src/cmd/destroy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,91 +23,98 @@ import (
"github.com/spf13/cobra"
)

var confirmDestroy bool
var removeComponents bool

var destroyCmd = &cobra.Command{
Use: "destroy --confirm",
Aliases: []string{"d"},
Short: lang.CmdDestroyShort,
Long: lang.CmdDestroyLong,
// TODO(mkcp): refactor and de-nest this function.
RunE: func(cmd *cobra.Command, _ []string) error {
ctx := cmd.Context()
l := logger.From(ctx)

timeoutCtx, cancel := context.WithTimeout(ctx, cluster.DefaultTimeout)
defer cancel()
c, err := cluster.NewClusterWithWait(timeoutCtx)
if err != nil {
return err
// ConnectOptions holds the command-line options for 'destroy' sub-command.
type DestroyOptions struct {
confirmDestroy bool
removeComponents bool
}

// NewDestroyCommand creates the `destroy` sub-command.
func NewDestroyCommand() *cobra.Command {
o := DestroyOptions{}
cmd := &cobra.Command{
Use: "destroy --confirm",
Aliases: []string{"d"},
Short: lang.CmdDestroyShort,
Long: lang.CmdDestroyLong,
RunE: o.Run,
}

// Still going to require a flag for destroy confirm, no viper oopsies here
cmd.Flags().BoolVar(&o.confirmDestroy, "confirm", false, lang.CmdDestroyFlagConfirm)
cmd.Flags().BoolVar(&o.removeComponents, "remove-components", false, lang.CmdDestroyFlagRemoveComponents)
_ = cmd.MarkFlagRequired("confirm")

return cmd
}

// Run performs the execution of 'destroy' sub-command.
func (o *DestroyOptions) Run(cmd *cobra.Command, _ []string) error {
ctx := cmd.Context()
l := logger.From(ctx)

timeoutCtx, cancel := context.WithTimeout(ctx, cluster.DefaultTimeout)
defer cancel()
c, err := cluster.NewClusterWithWait(timeoutCtx)
if err != nil {
return err
}

// NOTE: If 'zarf init' failed to deploy the k3s component (or if we're looking at the wrong kubeconfig)
// there will be no zarf-state to load and the struct will be empty. In these cases, if we can find
// the scripts to remove k3s, we will still try to remove a locally installed k3s cluster
state, err := c.LoadZarfState(ctx)
if err != nil {
message.WarnErr(err, err.Error())
l.Warn(err.Error())
}

// If Zarf deployed the cluster, burn it all down
if state.ZarfAppliance || (state.Distro == "") {
// Check if we have the scripts to destroy everything
fileInfo, err := os.Stat(config.ZarfCleanupScriptsPath)
if errors.Is(err, os.ErrNotExist) || !fileInfo.IsDir() {
return fmt.Errorf("unable to find the folder %s which has the scripts to cleanup the cluster. Please double-check you have the right kube-context", config.ZarfCleanupScriptsPath)
}

// NOTE: If 'zarf init' failed to deploy the k3s component (or if we're looking at the wrong kubeconfig)
// there will be no zarf-state to load and the struct will be empty. In these cases, if we can find
// the scripts to remove k3s, we will still try to remove a locally installed k3s cluster
state, err := c.LoadZarfState(ctx)
// Run all the scripts!
pattern := regexp.MustCompile(`(?mi)zarf-clean-.+\.sh$`)
scripts, err := helpers.RecursiveFileList(config.ZarfCleanupScriptsPath, pattern, true)
if err != nil {
message.WarnErr(err, err.Error())
l.Warn(err.Error())
return err
}

// If Zarf deployed the cluster, burn it all down
if state.ZarfAppliance || (state.Distro == "") {
// Check if we have the scripts to destroy everything
fileInfo, err := os.Stat(config.ZarfCleanupScriptsPath)
if errors.Is(err, os.ErrNotExist) || !fileInfo.IsDir() {
return fmt.Errorf("unable to find the folder %s which has the scripts to cleanup the cluster. Please double-check you have the right kube-context", config.ZarfCleanupScriptsPath)
// Iterate over all matching zarf-clean scripts and exec them
for _, script := range scripts {
// Run the matched script
err := exec.CmdWithPrint(script)
if errors.Is(err, os.ErrPermission) {
message.Warnf(lang.CmdDestroyErrScriptPermissionDenied, script)
l.Warn("received 'permission denied' when trying to execute script. Please double-check you have the correct kube-context.", "script", script)

// Don't remove scripts we can't execute so the user can try to manually run
continue
} else if err != nil {
return fmt.Errorf("received an error when executing the script %s: %w", script, err)
}

// Run all the scripts!
pattern := regexp.MustCompile(`(?mi)zarf-clean-.+\.sh$`)
scripts, err := helpers.RecursiveFileList(config.ZarfCleanupScriptsPath, pattern, true)
// Try to remove the script, but ignore any errors and debug log them
err = os.Remove(script)
if err != nil {
return err
}
// Iterate over all matching zarf-clean scripts and exec them
for _, script := range scripts {
// Run the matched script
err := exec.CmdWithPrint(script)
if errors.Is(err, os.ErrPermission) {
message.Warnf(lang.CmdDestroyErrScriptPermissionDenied, script)
l.Warn("received 'permission denied' when trying to execute script. Please double-check you have the correct kube-context.", "script", script)

// Don't remove scripts we can't execute so the user can try to manually run
continue
} else if err != nil {
return fmt.Errorf("received an error when executing the script %s: %w", script, err)
}

// Try to remove the script, but ignore any errors and debug log them
err = os.Remove(script)
if err != nil {
message.WarnErr(err, fmt.Sprintf("Unable to remove script. script=%s", script))
l.Warn("unable to remove script", "script", script, "error", err.Error())
}
message.WarnErr(err, fmt.Sprintf("Unable to remove script. script=%s", script))
l.Warn("unable to remove script", "script", script, "error", err.Error())
}
} else {
// Perform chart uninstallation
helm.Destroy(ctx, removeComponents)

// If Zarf didn't deploy the cluster, only delete the ZarfNamespace
if err := c.DeleteZarfNamespace(ctx); err != nil {
return err
}

// Remove zarf agent labels and secrets from namespaces Zarf doesn't manage
c.StripZarfLabelsAndSecretsFromNamespaces(ctx)
}
return nil
},
}
} else {
// Perform chart uninstallation
helm.Destroy(ctx, o.removeComponents)

func init() {
rootCmd.AddCommand(destroyCmd)
// If Zarf didn't deploy the cluster, only delete the ZarfNamespace
if err := c.DeleteZarfNamespace(ctx); err != nil {
return err
}

// Still going to require a flag for destroy confirm, no viper oopsies here
destroyCmd.Flags().BoolVar(&confirmDestroy, "confirm", false, lang.CmdDestroyFlagConfirm)
destroyCmd.Flags().BoolVar(&removeComponents, "remove-components", false, lang.CmdDestroyFlagRemoveComponents)
_ = destroyCmd.MarkFlagRequired("confirm")
// Remove zarf agent labels and secrets from namespaces Zarf doesn't manage
c.StripZarfLabelsAndSecretsFromNamespaces(ctx)
}
return nil
}
Loading
Loading