CLOUDP-56480: Clusters list command should return all clusters #45

andreaangiolillo · 2020-03-12T16:11:35Z

andreaangiolillo · 2020-03-12T16:34:31Z

internal/cli/cloud_manager_clusters_list.go

@@ -23,23 +23,26 @@ import (
 	"github.com/spf13/cobra"
 )

-type cmClustersListOpts struct {
+type clustersListOpts struct {


I decided to change the name since this is also used by OM

if you want to rename this do it to cloudManagerClustersListOpts I'm not that worried about it being shared as they are both the same, at some point ops-manager was an alias for cloud-manager but we were asked to give it its own space
We may eventually rename all to opsManager and files along or like we discussed find a new way to organize these commands (folders/packages?) so feel free of start thinking on those

andreaangiolillo · 2020-03-12T16:34:56Z

internal/cli/cloud_manager_clusters_list_test.go

 	if err != nil {
 		t.Fatalf("Run() unexpected error: %v", err)
 	}
 }
+
+func TestCloudManagerAllClustersProjectsList_Run(t *testing.T) {


Should we change the name since this is also used by only OM?

gssbzn

There's a lot to be done but it all comes from defining two stores when you only need one
Also let's not do atlas until we have that support in the atlas client

gssbzn · 2020-03-12T16:29:24Z

internal/cli/atlas_clusters_list.go

@@ -27,22 +27,25 @@ type atlasClustersListOpts struct {
 	*globalOpts
 	pageNum      int
 	itemsPerPage int
-	store        store.ClusterLister
+	storeAT      store.ClusterLister


there's no need to create a new store, just create a new interface that defines the two methods you need

gssbzn · 2020-03-12T16:31:07Z

internal/cli/atlas_clusters_list.go

 	if opts.ProjectID() == "" {
-		return errMissingProjectID
+		opts.storeOM, err = store.New()


store.New() implements all the methods of available this if is big code smell that you probably just need a new contract atlasClustersListOpts and not multiple fields, see my previous comment

gssbzn · 2020-03-12T16:32:14Z

internal/cli/atlas_clusters_list.go

 	return err
 }

-func (opts *atlasClustersListOpts) Run() error {
+func (opts *atlasClustersListOpts) RunAT() error {


this all comes from the same thing, multiple properties instead of a more flexible contract, there's no need to have multiple Run methods

gssbzn · 2020-03-12T16:33:51Z

internal/cli/atlas_clusters_list_test.go

 	if err != nil {
 		t.Fatalf("Run() unexpected error: %v", err)
 	}
 }
+
+func TestAtlasAllClustersProjectsList_Run(t *testing.T) {


you don't need a new test function, you should just define a new scenario
this block should be able to live in the already existing function

t.Run("List all clusters projects", func(t *testing.T) {...}

gssbzn · 2020-03-12T16:36:35Z

internal/store/all_clusters_projects.go

+	"github.com/mongodb/mongocli/internal/config"
+)
+
+type ListAllClusters interface {


it's a go convention to name single method interfaces as er adjectives, this should be AllClustersLister

gssbzn · 2020-03-12T16:37:05Z

internal/store/all_clusters_projects.go

+// ListAllClustersProjects encapsulate the logic to manage different cloud providers
+func (s *Store) ListAllClustersProjects() (*om.AllClustersProjects, error) {
+	switch s.service {
+	case config.OpsManagerService:


you should support CloudManagerService as well as is the same API

gssbzn

This looks so much better now, all code is good, let's just fix the formatting of the imports on the test file

gssbzn · 2020-03-13T09:52:05Z

internal/cli/cloud_manager_clusters_list.go

+		result, err = opts.store.ListAllClustersProjects()
+
+	} else {
+		var clusterConfigs *om.AutomationConfig


[np] this could be a function but feel free to ignore unless you want to do other changes

gssbzn · 2020-03-13T09:52:20Z

internal/cli/cloud_manager_clusters_list.go

+func (opts *cloudManagerClustersListOpts) Run() error {
+
+	var result interface{}
+	var err error


good use of a programatic error 👍

gssbzn · 2020-03-13T09:53:16Z

internal/cli/cloud_manager_clusters_list_test.go


 	defer ctrl.Finish()

-	expected := fixtures.AutomationConfig()
+	t.Run("ProjectID is given", func(t *testing.T) {


brilliant 💯

gssbzn · 2020-03-13T09:54:34Z

internal/cli/cloud_manager_clusters_list_test.go

@@ -17,32 +17,59 @@ package cli
 import (
 	"testing"

+	"github.com/mongodb/mongocli/internal/config"
+


small thing, extra empty line, let's remove it and fmtimport this file

gssbzn

LGTM

andreaangiolillo added 4 commits March 12, 2020 10:54

Starting implementing cluster list command

f1cce52

Merge remote-tracking branch 'origin' into CLOUDP-56480

16524e1

CLOUDP-56480: Clusters list command should return all clusters

f350cd6

Reverting commit : Starting implementing cluster list command

ec2d5cc

andreaangiolillo added the WIP Work in progress label Mar 12, 2020

andreaangiolillo added 3 commits March 12, 2020 16:14

Merge remote-tracking branch 'origin/master' into CLOUDP-56480

71a4332

Updated go.mod

3d4fa78

Refactoring

b544509

andreaangiolillo commented Mar 12, 2020

View reviewed changes

andreaangiolillo requested a review from gssbzn March 12, 2020 16:37

andreaangiolillo removed the WIP Work in progress label Mar 12, 2020

gssbzn requested changes Mar 12, 2020

View reviewed changes

andreaangiolillo added 2 commits March 12, 2020 18:30

Addressed PR Comments

9090190

Refactoring

cae7df6

andreaangiolillo requested a review from gssbzn March 12, 2020 18:41

gssbzn requested changes Mar 13, 2020

View reviewed changes

andreaangiolillo added 2 commits March 13, 2020 10:30

Addressed PR Comments

495464a

Refactoring

b5949fb

andreaangiolillo requested a review from gssbzn March 13, 2020 10:36

gssbzn approved these changes Mar 13, 2020

View reviewed changes

Merge branch 'master' into CLOUDP-56480

9ab485f

andreaangiolillo merged commit 1b56d3d into master Mar 13, 2020

andreaangiolillo deleted the CLOUDP-56480 branch March 13, 2020 11:04

CLOUDP-56480: Clusters list command should return all clusters #45

CLOUDP-56480: Clusters list command should return all clusters #45

Uh oh!

Conversation

andreaangiolillo commented Mar 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gssbzn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gssbzn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gssbzn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!