-
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
pkg/asset/installconfig: Add input validation #350
pkg/asset/installconfig: Add input validation #350
Conversation
b1aeb39
to
362eda8
Compare
"us-east-1": "N. Virginia", | ||
"us-east-2": "Ohio", | ||
"us-west-1": "N. California", | ||
"us-west-2": "Oregon", |
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 looks like something that could be a pain to keep up to date. How about a TODO or something to consider getting the list from an AWS API?
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.
The installer doesn't require Internet access today. Reading this list from AWS would introduce that dependency.
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.
How about a TODO or something to consider getting the list from an AWS API?
And also, AWS doesn't seem to provide an API for pulling the locations. Previous discussion starting here.
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.
ok, thanks for the info and references
Options: validPlatforms, | ||
}, | ||
Validate: survey.ComposeValidators(survey.Required, func(ans interface{}) error { | ||
choice := ans.(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.
Put a check here? Just in case the dynamic type cast fails.
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.
No, this is correct. Programs should panic if there is a programmer error (vs an input error).
pkg/asset/installconfig/platform.go
Outdated
@@ -114,16 +147,26 @@ func (a *Platform) awsPlatform() (*asset.State, error) { | |||
|
|||
return assetStateForStringContents( | |||
AWSPlatformType, | |||
strings.Split(string(region.Contents[0].Data), " ")[0], | |||
strings.SplitN(string(region.Contents[0].Data), " ", 2)[0], |
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.
Let's use a transformation (provided by Survey) instead of manipulating the user's choice.
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.
Let's use a transformation (provided by Survey) instead of manipulating the user's choice.
This sounded good, but turned out to be not a big win (I've done it in 362eda8 -> 5b3c205). It's nice to not have to muck with the response before we return it, but the validator fires before the tranform, so the validator needs to be able to handle both the short and long forms. I dunno why they didn't transform before validating...
Looks good! Just a few questions/suggestions. |
By switching to survey.Question which has room for a validator [1]. I've also pushed the Required validator up into the per-property config, so we don't bake requiredness into UserProvided itself. [1]: https://godoc.org/github.com/AlecAivazis/survey#Question
362eda8
to
5b3c205
Compare
}, | ||
Validate: survey.ComposeValidators(survey.Required, func(ans interface{}) error { | ||
choice := regionTransform(ans).(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.
Is this cast needed? regionTransform()
returns a 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.
Is this cast needed?
Yes.
regionTransform()
returns a string.
No, regionTransform
holds the result of TransformString
, which is a Tranformer
(and returns an interface{}
). The string return is the function we're passing into TransformString
as an argument, and we don't assign that function a name.
/lgtm Go ahead and cancel the hold if my last concern can be ignored. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: crawford, wking 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 |
/hold cancel |
By switching to
survey.Question
which has room for a validator. I've also pushed theRequired
validator up into the per-property config, so we don't bake requiredness intoUserProvided
itself./assign @crawford