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

Remove the APIClient interface and refactor the CLI #1870

Closed
echlebek opened this issue Jul 25, 2018 · 1 comment
Closed

Remove the APIClient interface and refactor the CLI #1870

echlebek opened this issue Jul 25, 2018 · 1 comment

Comments

@echlebek
Copy link
Contributor

echlebek commented Jul 25, 2018

The client package has a god-interface with an ever-growing method set. It's causing us some problems.

Refactor the client so that this interface is not required.

  1. Refactor each command to not require a *cli.SensuCli. Instead it should require a config object and a minimal interface. For example, here is the diff for refactoring the check create command:
diff --git a/cli/commands/check/create.go b/cli/commands/check/create.go
index 4ea65ee2..38958087 100644
--- a/cli/commands/check/create.go
+++ b/cli/commands/check/create.go
@@ -4,7 +4,8 @@ import (
        "errors"
        "fmt"
 
-       "github.com/sensu/sensu-go/cli"
+       "github.com/sensu/sensu-go/cli/client"
+       "github.com/sensu/sensu-go/cli/client/config"
        "github.com/sensu/sensu-go/cli/commands/flags"
        "github.com/sensu/sensu-go/cli/commands/helpers"
        "github.com/sensu/sensu-go/types"
@@ -12,7 +13,7 @@ import (
 )
 
 // CreateCommand adds command that allows user to create new checks
-func CreateCommand(cli *cli.SensuCli) *cobra.Command {
+func CreateCommand(cfg config.Config, client client.CheckAPIClient) *cobra.Command {
        cmd := &cobra.Command{
                Use:          "create [NAME]",
                Short:        "create new checks",
@@ -38,8 +39,8 @@ func CreateCommand(cli *cli.SensuCli) *cobra.Command {
                                opts.Name = args[0]
                        }
 
-                       opts.Org = cli.Config.Organization()
-                       opts.Env = cli.Config.Environment()
+                       opts.Org = cfg.Organization()
+                       opts.Env = cfg.Environment()
 
                        if isInteractive {
                                if err := opts.administerQuestionnaire(false); err != nil {
@@ -73,7 +74,7 @@ func CreateCommand(cli *cli.SensuCli) *cobra.Command {
                        // determine whether there are assets / handlers / mutators associated w/
                        // the check and warn the user if they do not exist yet.
 
-                       if err := cli.Client.CreateCheck(&check); err != nil {
+                       if err := client.CreateCheck(&check); err != nil {
                                return err
                        }
 
diff --git a/cli/commands/check/create_test.go b/cli/commands/check/create_test.go
index 4fc983b7..eeb3d95f 100644
--- a/cli/commands/check/create_test.go
+++ b/cli/commands/check/create_test.go
@@ -15,7 +15,7 @@ func TestCreateCommand(t *testing.T) {
        assert := assert.New(t)
 
        cli := test.NewMockCLI()
-       cmd := CreateCommand(cli)
+       cmd := CreateCommand(cli.Config, cli.Client)
 
        assert.NotNil(cmd, "cmd should be returned")
        assert.NotNil(cmd.RunE, "cmd should be able to be executed")
@@ -27,7 +27,7 @@ func TestCreateCommandRunEClosureWithoutFlags(t *testing.T) {
        assert := assert.New(t)
 
        cli := test.NewMockCLI()
-       cmd := CreateCommand(cli)
+       cmd := CreateCommand(cli.Config, cli.Client)
        require.NoError(t, cmd.Flags().Set("interval", "sdfsa"))
        out, err := test.RunCmd(cmd, []string{"echo 'heyhey'"})
 
@@ -42,7 +42,7 @@ func TestCreateCommandRunEClosureWithAllFlags(t *testing.T) {
        client := cli.Client.(*client.MockClient)
        client.On("CreateCheck", mock.AnythingOfType("*types.CheckConfig")).Return(nil)
 
-       cmd := CreateCommand(cli)
+       cmd := CreateCommand(cli.Config, cli.Client)
        require.NoError(t, cmd.Flags().Set("command", "echo 'heyhey'"))
        require.NoError(t, cmd.Flags().Set("subscriptions", "system"))
        require.NoError(t, cmd.Flags().Set("interval", "10"))
@@ -59,7 +59,7 @@ func TestCreateCommandRunEClosureWithDeps(t *testing.T) {
        client := cli.Client.(*client.MockClient)
        client.On("CreateCheck", mock.AnythingOfType("*types.CheckConfig")).Return(nil)
 
-       cmd := CreateCommand(cli)
+       cmd := CreateCommand(cli.Config, cli.Client)
        require.NoError(t, cmd.Flags().Set("command", "echo 'heyhey'"))
        require.NoError(t, cmd.Flags().Set("subscriptions", "system"))
        require.NoError(t, cmd.Flags().Set("interval", "10"))
@@ -77,7 +77,7 @@ func TestCreateCommandRunEClosureWithDepsSTDIN(t *testing.T) {
        client := cli.Client.(*client.MockClient)
        client.On("CreateCheck", mock.AnythingOfType("*types.CheckConfig")).Return(nil)
 
-       cmd := CreateCommand(cli)
+       cmd := CreateCommand(cli.Config, cli.Client)
        require.NoError(t, cmd.Flags().Set("command", "echo 'heyhey'"))
        require.NoError(t, cmd.Flags().Set("subscriptions", "system"))
        require.NoError(t, cmd.Flags().Set("interval", "10"))
@@ -96,7 +96,7 @@ func TestCreateCommandRunEClosureWithServerErr(t *testing.T) {
        client := cli.Client.(*client.MockClient)
        client.On("CreateCheck", mock.AnythingOfType("*types.CheckConfig")).Return(errors.New("whoops"))
 
-       cmd := CreateCommand(cli)
+       cmd := CreateCommand(cli.Config, cli.Client)
        require.NoError(t, cmd.Flags().Set("command", "echo 'heyhey'"))
        require.NoError(t, cmd.Flags().Set("subscriptions", "system"))
        require.NoError(t, cmd.Flags().Set("interval", "10"))
@@ -114,7 +114,7 @@ func TestCreateCommandRunEClosureWithMissingRequiredFlags(t *testing.T) {
        client := cli.Client.(*client.MockClient)
        client.On("CreateCheck", mock.AnythingOfType("*types.CheckConfig")).Return(errors.New("error"))
 
-       cmd := CreateCommand(cli)
+       cmd := CreateCommand(cli.Config, cli.Client)
        require.NoError(t, cmd.Flags().Set("interval", "5"))
        out, err := test.RunCmd(cmd, []string{"checky"})
        require.Error(t, err)
diff --git a/cli/commands/check/help.go b/cli/commands/check/help.go
index c39ec059..b908b4f7 100644
--- a/cli/commands/check/help.go
+++ b/cli/commands/check/help.go
@@ -16,7 +16,7 @@ func HelpCommand(cli *cli.SensuCli) *cobra.Command {
        // Add sub-commands
        cmd.AddCommand(
                // CRUD Commands
-               CreateCommand(cli),
+               CreateCommand(cli.Config, cli.Client),
                DeleteCommand(cli),
                ExecuteCommand(cli),
                ListCommand(cli),
  1. Refactor the root and help commands to work this way too.

  2. Refactor the main to work this way (./cmd/sensuctl)

  3. Remove the cli.APIClient interface.

When the refactor is complete, the cli.SensuCli type will only be used in sensuctl main.

@stale
Copy link

stale bot commented Dec 8, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Dec 8, 2020
@stale stale bot closed this as completed Dec 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants