Various cleanups around apply_cluster and awsmodel #10579
Conversation
k8s-ci-robot
added
kind/cleanup
k8s-ci-robot
added
area/provider/spotinst
olemarkus
force-pushed
the
refactor-apply-cluster
branch
from
6a24792
to
54db39f
Compare
k8s-ci-robot
removed
the
needs-rebase
|
/test pull-kops-e2e-kubernetes-aws |
k8s-ci-robot
added
the
needs-rebase
olemarkus
force-pushed
the
refactor-apply-cluster
branch
from
54db39f
to
c2a048c
Compare
k8s-ci-robot
removed
the
needs-rebase
|
As discussed during Office Hours, Justin will give the final approval on this. |
| @@ -67,7 +66,7 @@ func (b *AutoscalingGroupModelBuilder) Build(c *fi.ModelBuilderContext) error { | |||
| name := b.AutoscalingGroupName(ig) | |||
|
|
|||
| if featureflag.SpotinstHybrid.Enabled() { | |||
| if spotinstmodel.HybridInstanceGroup(ig) { | |||
| if HybridInstanceGroup(ig) { | |||
There was a problem hiding this comment.
We might want to call this SpotInstHybridInstanceGroup if it's spotinst specific
| @@ -270,11 +271,11 @@ func (b *BastionModelBuilder) Build(c *fi.ModelBuilderContext) error { | |||
| // Here we implement the bastion CNAME logic | |||
| // By default bastions will create a CNAME that follows the `bastion-$clustername` formula | |||
| t := &awstasks.DNSName{ | |||
| Name: s(bastionPublicName), | |||
| Name: fi.String(bastionPublicName), | |||
There was a problem hiding this comment.
I think I agree with this change (it's more verbose but clearer not to use the helper functions), but it might have been easier to do this as a follow-on PR or separate commit, just from a change readability perspective.
There was a problem hiding this comment.
Yeah. This change was mainly done because awstasks does not have these helper functions, and I didn't see the point of copying them.
|
One suggestion to rename a spotinst-specific method ("HybridInstanceGroup") but non-blocking and this is good /approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hakman, justinsb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Review the full test history for this PR. Silence the bot with an |
olemarkus
force-pushed
the
refactor-apply-cluster
branch
from
c2a048c
to
8a20572
Compare
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
…579-origin-release-1.20 Automated cherry pick of #10579: Set ssh key on the model context in one location
A lot of the AWS specific builders exist in the generic
modelpackage. Most of them have now been moved toawsmodel.Some are left behind because they are (ab)used elsewhere in kOps.
Also merged spotinst builder into
awsmodelbecause spotinst isn't a true cloud provider and there are cyclic dependencies between the two.Also merged the two large switch statements.
/kind cleanup