-
Notifications
You must be signed in to change notification settings - Fork 82
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
accept amitype in kubetest2 managed nodegroup creation #438
Conversation
I think we should add validation that a |
80e28c7
to
605d7e4
Compare
if d.AMIType != "" && d.AMI != "" { | ||
return fmt.Errorf("Cannot accept mutually exclusive flags --ami-type and --ami because it implies conflicting deployment modes") |
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.
need some suggestion on this message/condition. we don't validate you can't provide the --ami
flag if you're not using unmanaged nodes, so not sure whether enforcing this is useful in practice @cartermckinnon
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.
I think it'd be nice to validate that too :) but it's fine to just say "these two flags are mutually exclusive" in the error message, the flag docs can explain when/how a flag is used
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.
validating each on their own according to --unmanaged-nodes
handles the mutual exclusion and i think is clearer. small update with this
Issue #, if available:
Description of changes:
add a flag to pass
ami-type
to the nodegroup creation for kubetest2 deployerBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.