-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Improve consistency of command functions #1124
Conversation
3c85303
to
1aa3879
Compare
1aa3879
to
8c28025
Compare
8c28025
to
876583b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a lot of repetition going on, could we possibly move these into functions?
eks.New()
could be something likeeks.NewWithRegion(…) (ctl, error)
- for config defaults and validation could add
cmdutils.ResourceCmd#GetDefaultedAndValidatedConfig(…) (cfg, error)
(i'm sure you'll come up with better names)
ed31a03
to
b9abc71
Compare
@rndstr thanks, indeed this is getting a little mad. I've added a method that wraps it up. |
b9abc71
to
f4b3b5c
Compare
- perform defaulting and validation in all commands - check region is supported
f4b3b5c
to
3f90efe
Compare
|
||
if !ctl.IsSupportedRegion() { | ||
return cmdutils.ErrUnsupportedRegion(rc.ProviderConfig) | ||
ctl, err := rc.NewCtl() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we pass a ClusterProvider
as an argument to command handlers like doCreateCluster
, instead of constructing it here, we can inject a ClusterProvider
with fake AWS API clients for api.ClusterProvider
. That'd let us unit-test command handlers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, but let's make that change separately. I'm just trying to take care of consistency here. I think that change will be easier to make now.
Actually, we could just make it happen inside of NewCtl
, i.e. make it possible to set the provider implementation in ResourceCmd
(which gets constructed before the handler function runs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to do it later in a separate PR.
api.SetNodeGroupDefaults(i, ng) | ||
} | ||
|
||
ctl := eks.New(rc.ProviderConfig, rc.ClusterConfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be something like:
if rc.MockProvider != nil {
return rc.MockProvider, nil
}
ctl := eks.New(rc.ProviderConfig, rc.ClusterConfig)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But let's leave until next iteration, I also have a PR to rename rc
to simply cmd
, which I promised to Martina a long time ago.
add ecr-private kustomize overlay
Description
A follow up to #1116.
Checklist
make build
)make test
)