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

chore: replace global variables with Config struct #26

Closed
wants to merge 2 commits into from
+21 −18
Closed

chore: replace global variables with Config struct #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2408d5b
add local.Config
colesnodgrass May 14, 2024
ae62801
remove telClient global variable
colesnodgrass May 14, 2024
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
2 changes: 1 addition & 1 deletion internal/cmd/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ func NewCmd() *cobra.Command {
cmd.PersistentFlags().BoolVarP(&flagVerbose, "verbose", "v", false, "enable verbose output")

cmd.AddCommand(version.NewCmdVersion())
cmd.AddCommand(local.NewCmdLocal(k8s.DefaultProvider))
cmd.AddCommand(local.NewCmdLocal(&local.Config{Provider: k8s.DefaultProvider}))

return cmd
}
14 changes: 8 additions & 6 deletions internal/cmd/local/local.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,13 @@ import (
"path/filepath"
)

var telClient telemetry.Client
type Config struct {
Provider k8s.Provider
TelClient telemetry.Client
}

// NewCmdLocal represents the local command.
func NewCmdLocal(provider k8s.Provider) *cobra.Command {
func NewCmdLocal(cfg *Config) *cobra.Command {
cmd := &cobra.Command{
Use: "local",
PersistentPreRunE: func(cmd *cobra.Command, _ []string) error {
Expand All @@ -24,17 +27,16 @@ func NewCmdLocal(provider k8s.Provider) *cobra.Command {
if dnt {
telOpts = append(telOpts, telemetry.WithDnt())
}

telClient = telemetry.Get(telOpts...)
cfg.TelClient = telemetry.Get(telOpts...)
}
printProviderDetails(provider)
printProviderDetails(cfg.Provider)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is where I find it useful to have a singleton Config struct, which can be constructed at the root command (or the root of the command subtree that makes use of those configs), as opposed to threading it through to all of these subcommand constructors.

I also thing its a little odd to take a config struct here, modify it by assigning the TelClient and then passing that along. So I would suggest assigning TelClient in the caller (parent command)


return nil
},
Short: "Manages local Airbyte installations",
}

cmd.AddCommand(NewCmdInstall(provider), NewCmdUninstall(provider))
cmd.AddCommand(newCmdInstall(cfg), newCmdUninstall(cfg))

return cmd
}
Expand Down
11 changes: 6 additions & 5 deletions internal/cmd/local/local_install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ const (
envBasicAuthPass = "ABCTL_LOCAL_INSTALL_PASSWORD"
)

func NewCmdInstall(provider k8s.Provider) *cobra.Command {
func newCmdInstall(cfg *Config) *cobra.Command {
spinner := &pterm.DefaultSpinner
provider := cfg.Provider

var (
flagChartVersion string
Expand All @@ -41,9 +42,9 @@ func NewCmdInstall(provider k8s.Provider) *cobra.Command {
return fmt.Errorf("could not determine docker installation status: %w", err)
}

telClient.Attr("docker_version", dockerVersion.Version)
telClient.Attr("docker_arch", dockerVersion.Arch)
telClient.Attr("docker_platform", dockerVersion.Platform)
cfg.TelClient.Attr("docker_version", dockerVersion.Version)
cfg.TelClient.Attr("docker_arch", dockerVersion.Arch)
cfg.TelClient.Attr("docker_platform", dockerVersion.Platform)

spinner.UpdateText(fmt.Sprintf("Checking if port %d is available", flagPort))
if err := portAvailable(cmd.Context(), flagPort); err != nil {
Expand Down Expand Up @@ -104,7 +105,7 @@ func NewCmdInstall(provider k8s.Provider) *cobra.Command {

lc, err := local.New(provider,
local.WithPortHTTP(flagPort),
local.WithTelemetryClient(telClient),
local.WithTelemetryClient(cfg.TelClient),
local.WithSpinner(spinner),
local.WithHelmChartVersion(flagChartVersion),
)
Expand Down
12 changes: 6 additions & 6 deletions internal/cmd/local/local_uninstall.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@ package local

import (
"fmt"
"github.com/airbytehq/abctl/internal/cmd/local/k8s"
"github.com/airbytehq/abctl/internal/cmd/local/local"
"github.com/airbytehq/abctl/internal/telemetry"
"github.com/pterm/pterm"
"github.com/spf13/cobra"
)

func NewCmdUninstall(provider k8s.Provider) *cobra.Command {
func newCmdUninstall(cfg *Config) *cobra.Command {
spinner := &pterm.DefaultSpinner
provider := cfg.Provider

cmd := &cobra.Command{
Use: "uninstall",
Expand All @@ -25,9 +25,9 @@ func NewCmdUninstall(provider k8s.Provider) *cobra.Command {
return fmt.Errorf("could not determine docker installation status: %w", err)
}

telClient.Attr("docker_version", dockerVersion.Version)
telClient.Attr("docker_arch", dockerVersion.Arch)
telClient.Attr("docker_platform", dockerVersion.Platform)
cfg.TelClient.Attr("docker_version", dockerVersion.Version)
cfg.TelClient.Attr("docker_arch", dockerVersion.Arch)
cfg.TelClient.Attr("docker_platform", dockerVersion.Platform)

return nil
},
Expand All @@ -49,7 +49,7 @@ func NewCmdUninstall(provider k8s.Provider) *cobra.Command {

pterm.Success.Printfln("Existing cluster '%s' found", provider.ClusterName)

lc, err := local.New(provider, local.WithTelemetryClient(telClient), local.WithSpinner(spinner))
lc, err := local.New(provider, local.WithTelemetryClient(cfg.TelClient), local.WithSpinner(spinner))
if err != nil {
pterm.Warning.Printfln("Failed to initialize 'local' command\nUninstallation attempt will continue")
pterm.Debug.Printfln("Initialization of 'local' failed with %s", err.Error())
Expand Down