Skip to content

Some refactoring on leo subcommands #664

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

Closed
wants to merge 14 commits into from
Closed

Conversation

tasdomas
Copy link
Contributor

@tasdomas tasdomas commented Sep 9, 2022

Renamed delete to destroy.
Renamed read to status.
Restructured base options and moved all subcommands into a single package.

Copy link
Member

@0x2b3bfa0 0x2b3bfa0 left a comment

Choose a reason for hiding this comment

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

moved all subcommands into a single package

Why? 🤔

@tasdomas
Copy link
Contributor Author

tasdomas commented Sep 9, 2022

moved all subcommands into a single package

Why? thinking

Is there any reason to keep identical single file packages?

@0x2b3bfa0
Copy link
Member

Is there any reason to keep identical single file packages?

“Namespaces are one honking great idea — let's do more of those!” Wrong language! 🤭

It probably boils down to a matter of taste1 but, to my mind, it looks cleaner once we introduce subcommands or more complex commands (e.g. #291 or #531) which may be split over several files.

See also docker/cli, kubernetes/kubectl or cli/cli for inspiration.

Footnotes

  1. Or lack thereof, you're the wise gopher here.

delete -> destroy
read -> status
It looks like there's a bug in cobra where a required persistent flag is enforced even for the help command.
@tasdomas tasdomas changed the title Some refactoring on leo subcommands. Some refactoring on leo subcommands Oct 4, 2022
Copy link
Member

@0x2b3bfa0 0x2b3bfa0 left a comment

Choose a reason for hiding this comment

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

I'd still prefer to keep subcommands in separate packages, unless there is a good reason not to.

Sample good reason: the structure you propose eliminates beautifully the common.Cloud “dependency injection” used by the previous approach.

Relevant XKCD

I'm not sure if this is actually true

@tasdomas tasdomas closed this Oct 10, 2022
Comment on lines +1 to +58
package cmd

import (
"time"

"github.com/sirupsen/logrus"
"github.com/spf13/cobra"
"github.com/spf13/pflag"

"terraform-provider-iterative/task/common"
)

// BaseOptions specify base flags for commands that interact with
// cloud deployments.
type BaseOptions struct {
Region string
Provider string
Verbose bool
}

// defaultCloud specifies default timeouts.
var defaultCloud = common.Cloud{
Timeouts: common.Timeouts{
Create: 15 * time.Minute,
Read: 3 * time.Minute,
Update: 3 * time.Minute,
Delete: 15 * time.Minute,
},
}

// SetFlags sets base option flags on the provided flagset.
func (o *BaseOptions) SetFlags(f *pflag.FlagSet) {
f.StringVar(&o.Provider, "cloud", "", "cloud provider")
f.StringVar(&o.Region, "region", "us-east", "cloud region")
f.BoolVar(&o.Verbose, "verbose", false, "verbose output")
cobra.CheckErr(cobra.MarkFlagRequired(f, "cloud"))
}

// GetCloud parses cloud-specific options and returns a cloud structure.
func (o *BaseOptions) GetCloud() *common.Cloud {
cloud := defaultCloud
cloud.Provider = common.Provider(o.Provider)
cloud.Region = common.Region(o.Region)
return &cloud
}

// ConfigureLogging configures logging and sets the log level.
func (o *BaseOptions) ConfigureLogging() {
logrus.SetLevel(logrus.InfoLevel)
if o.Verbose {
logrus.SetLevel(logrus.DebugLevel)
}

logrus.SetFormatter(&logrus.TextFormatter{
ForceColors: true,
DisableTimestamp: true,
})
}
Copy link
Member

Choose a reason for hiding this comment

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

Message in a botte for myself: lots of good stuff here, don't let them disappear forever

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

Successfully merging this pull request may close these issues.

2 participants