-
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
Clear separation between api.ClusterConfig
and api.ClusterProvider
#309
Conversation
6383198
to
cc8ef80
Compare
d414e20
to
c5aa0ae
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.
Few things to tidy up...
12dfa42
to
e322a96
Compare
api.ClusterConfig
and api.ClusterProvider
api.ClusterConfig
and api.ClusterProvider
34d42a5
to
a34efcb
Compare
tags []*cloudformation.Tag | ||
cfn cloudformationiface.CloudFormationAPI | ||
waitTimeout time.Duration | ||
spec *api.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.
We still keep a pointer here, but it's arguably more sensible, as there aren't many different kinds of calls to stack manager from the outside, so it's sensible to set it once. It basically gets handed off at this point, and the stack manager is coupled the cluster itself, there isn't a good reason to have it decoupled at this point, unlike the instance of the provider.
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 generally looks good to me, great work. Just a couple of comments.
type ClusterProvider interface { | ||
CloudFormation() cloudformationiface.CloudFormationAPI | ||
EKS() eksiface.EKSAPI | ||
EC2() ec2iface.EC2API | ||
STS() stsiface.STSAPI | ||
Region() string |
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.
We may need some guidance for contributors when to split out / duplicate fields like we have done with region
. I get why its done but its probably something we need to keep an eye on just in case there is a proliferation .
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.
Yes, I agree. I wanted to avoid Profile()
here, but it's used somewhere and I didn't manage to get rid of it in a sensible manner. We need some guidance here indeed.
a34efcb
to
cf310af
Compare
This refactors how we separate provider configuration and it's recievers from cluster configuration. We no longer copy cluster configuration in every places where we need it, instead we pass it explicitly every time.
cf310af
to
e86ded8
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.
LGTM!
Description
This refactors how we separate provider configuration and it's recievers
from cluster configuration. We no longer copy cluster configuration in
every places where we need it, instead we pass it explicitly every time.
Checklist
make build
)make test
)humans.txt
file