Refactor command functions #859

errordeveloper · 2019-06-07T15:49:01Z

Description

create short-hand methods for defining standard commands
move most of common parameters to a struct
get rid of global state
improve overall consistency of command functions
close add common structs for command options #740
also a step towards Import eksctl as a library to integrate other utils #813

Checklist

Code compiles correctly (i.e make build)
Added tests that cover your change (if possible)
All unit tests passing (i.e. make test)
All integration tests passing (i.e. make integration-test)

pkg/ctl/cmdutils/resource.go

martina-if · 2019-06-10T13:10:00Z

pkg/ctl/create/cluster.go

 	cfg := api.NewClusterConfig()
 	ng := cfg.NewNodeGroup()
+	rc.ClusterConfig = cfg


Since there is no other command in this context can we rename rc to cmd or command which is easier to understand what is is?

It's ResourceCmd, hence rc. Using resourceCmd would be too verbose IMO. Calling it resource is also kind of misleading, it's not really a resource. Maybe we could rename the struct? But I've already tried a bunch of names, and this is the only one that sticks... Anyhow, to me this PR is only start of the process, I'd rather go on to next thing and revisit this later.

We will soon need to get more refactoring done and decouple things into a library that doesn't have any assumptions about being a used in CLI, so we will have revisit this. I suspect we might turn ResourceCmd into an interface that will have CLI-oriented implementation, and a more general one also. But it's a little too early to decide on that.

Why not just command or cmd? it is a resourceCmd but there could be a similar actionCmd or similar and they would all be commands. What do you think?

martina-if · 2019-06-10T13:29:33Z

pkg/ctl/create/iamidentitymapping.go

@@ -14,45 +11,40 @@ import (
 	"github.com/weaveworks/eksctl/pkg/eks"
 )

-func createIAMIdentityMappingCmd(g *cmdutils.Grouping) *cobra.Command {
-	p := &api.ProviderConfig{}
+func createIAMIdentityMappingCmd(rc *cmdutils.ResourceCmd) {


If all these create*Cmd() functions manipulate the first parameter, ResourceCmd, I think it would be more idiomatic if it was the receiver of these methods. Since I don't normally expect a parameter of a function to be mutated by it. What do you think?

it would be more idiomatic if it was the receiver of these methods

yes, but that means all those have to be in the same package...

Since I don't normally expect a parameter of a function to be mutated by it.

It's unfortunate, but we already have too many places where *api.ClusterConfig is passed in and mutated, which is kind of by design. We can discuss how that can be fix, but I'm not entirely sure if it the most important thing at the moment. (Only if there was a way in Go to prevent mutation!)

martina-if · 2019-06-10T13:50:01Z

pkg/ctl/create/iamidentitymapping.go


-	group.InFlagSet("General", func(fs *pflag.FlagSet) {
+	rc.AddCommand(func() error {
+		return doCreateIAMIdentityMapping(rc, id)


I'm not sure about this cyclic dependency here: rc -> doCreateIAMIdentityMapping -> rc. I need to think about this

martina-if · 2019-06-10T14:10:10Z

pkg/ctl/utils/describe_stacks.go

@@ -82,11 +68,11 @@ func doDescribeStacksCmd(p *api.ProviderConfig, cfg *api.ClusterConfig, nameArg
 	}

 	for _, s := range stacks {
-		if !describeStacksAll && *s.StackStatus == cloudformation.StackStatusDeleteComplete {


I think the old names here were more clear, I would leave them as they were.

martina-if · 2019-06-10T14:20:19Z

Nice refactor! And I think it's great that we also rename other things like p to provider. I would rename the rc to something else that's more clear. Maybe something like cmd, command, action, context or something like that, because today, there are a lot of things with very short cryptic names like ctl, fs and it would be nice, specially if we get more contributors, to make the code more explicit and easy to read.

- create short-hand methods for defining standard commands - move most of common parameters to a struct - get rid of global state - improve overall consistency of command functions

errordeveloper · 2019-06-10T16:23:55Z

I've renamed a few more things, name got rid of g (it's flagGrouping now). Let's leave rc as is for now, and revisit later. I agree it could have a better name, but I've been overthinking it for too long already tbh, I think it's the name of struct, but as I said we need should try to think of librarisation next, and I'm sure something better will come up in the process!

martina-if

If you say we will get rid of the rc in the next PRs we can merge this

errordeveloper · 2019-06-11T07:33:45Z

@martina-if yeah, next PR towards #813.

errordeveloper mentioned this pull request Jun 7, 2019

Introduce structure that holds common parameters for most of command #856

Closed

4 tasks

errordeveloper requested a review from martina-if June 7, 2019 15:49

errordeveloper force-pushed the refactor-command-functions branch 2 times, most recently from e1f6e02 to a82c616 Compare June 7, 2019 16:07

errordeveloper marked this pull request as ready for review June 10, 2019 07:32

martina-if reviewed Jun 10, 2019

View reviewed changes

Refactor command functions

dbedfd1

- create short-hand methods for defining standard commands - move most of common parameters to a struct - get rid of global state - improve overall consistency of command functions

errordeveloper force-pushed the refactor-command-functions branch from a82c616 to dbedfd1 Compare June 10, 2019 16:15

martina-if approved these changes Jun 11, 2019

View reviewed changes

errordeveloper merged commit 3711dbf into master Jun 11, 2019

errordeveloper deleted the refactor-command-functions branch June 11, 2019 07:33

This was referenced Jun 11, 2019

Re-introduce name arg where it was previously allowed #867

Merged

default value of boolean flags changed #870

Closed

errordeveloper mentioned this pull request Aug 9, 2019

Rename cmdutils.ResourceCmd to cmdutils.Cmd #1127

Merged

2 tasks

2opremio mentioned this pull request Aug 20, 2019

get nodegroups command no longer display all nodegroups #1164

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor command functions #859

Refactor command functions #859

errordeveloper commented Jun 7, 2019 •

edited

Loading

martina-if Jun 10, 2019

errordeveloper Jun 10, 2019

martina-if Jun 11, 2019

martina-if Jun 10, 2019

errordeveloper Jun 10, 2019

errordeveloper Jun 10, 2019

martina-if Jun 10, 2019

martina-if Jun 10, 2019

martina-if commented Jun 10, 2019

errordeveloper commented Jun 10, 2019 •

edited

Loading

martina-if left a comment

errordeveloper commented Jun 11, 2019

Refactor command functions #859

Refactor command functions #859

Conversation

errordeveloper commented Jun 7, 2019 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martina-if commented Jun 10, 2019

errordeveloper commented Jun 10, 2019 • edited Loading

martina-if left a comment

Choose a reason for hiding this comment

errordeveloper commented Jun 11, 2019

errordeveloper commented Jun 7, 2019 •

edited

Loading

errordeveloper commented Jun 10, 2019 •

edited

Loading