Improve cluster config loader #671
Conversation
errordeveloper
force-pushed
the
cluster-config-loader
branch
from
14b3a7d to
9370121
Compare
| // ImageFamilyAmazonLinux2 represents Amazon Linux 2 family | ||
| ImageFamilyAmazonLinux2 = "AmazonLinux2" // Owner 602401143452 | ||
| ImageFamilyAmazonLinux2 = api.NodeImageFamilyAmazonLinux2 // Owner 602401143452 |
There was a problem hiding this comment.
The constants are being relocated to api package, so that they can be used there without dragging AWS dependencies from here, as the api package is meant to be free of dependencies.
| @@ -0,0 +1,56 @@ | |||
| package v1alpha4 | |||
|
|
|||
| // SetNodeGroupDefaults will set defaults for a given nodegroup | |||
There was a problem hiding this comment.
This wasn't moved here before due to the concerns regarding ami package and its dependencies.
There was a problem hiding this comment.
Why not keep this in the nodegroup_filter package as private and only expose ValidateNodeGroupsAndSetDefaults()? Since this is less abstracted and not really useful by itself, or is it? :/
There was a problem hiding this comment.
So NodeGroupFilter is currently designed for CLI use-cases, hence it lives in cmduitls.
This is here so that one can construct ClusterConfig, and apply defaults and validation in a deamon/operator context or whatever other context. I'm just trying to draw a clear line between CLI and API.
These are only meant for interactive use in commands, not the API. So cmdutils is the best place to keep these.
- make most logic part of the API - move contstanst out of ami package - convert subnet checker into a method - make `SkipAll` an option of the `NodeGroupFilter`
errordeveloper
force-pushed
the
cluster-config-loader
branch
from
9370121 to
5cbb187
Compare
errordeveloper
changed the title
This is something that can happen due to the fact that defaulting is now done for both case - config file and flag.
| } | ||
|
|
||
| // Enable SSH when a key is provided | ||
| if ng.SSHPublicKeyPath != "" && ng.SSHPublicKeyPath != DefaultNodeSSHPublicKeyPath { |
There was a problem hiding this comment.
@martina-if this is a temporary fix, I think we should be able to get rid of != DefaultNodeSSHPublicKeyPath when we switch to references.
…`doCreateCluster`
Description
Checklist
make build)make test)make integration-test)