Skip to content
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

allows parameter types to be case-insensitive #599

Merged
merged 1 commit into from Dec 19, 2017

Conversation

mhrivnak
Copy link
Member

The error message when failing to parse a spec is improved to include the name
of the spec.

fixes #592

The error message when failing to parse a spec is improved to include the name
of the spec.

fixes openshift#592
@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 15, 2017
@coveralls
Copy link

coveralls commented Dec 15, 2017

Coverage Status

Changes Unknown when pulling 30e6ad3 on mhrivnak:lowercase-param-types into ** on openshift:master**.

Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Contributor

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that we don't have great test coverage but this is a case where we could add one.

@@ -196,7 +197,7 @@ func createUIFormItem(pd apb.ParameterDescriptor, paramIndex int) (interface{},

// getType transforms an apb parameter type to a JSON Schema type
func getType(paramType string) (schema.PrimitiveTypes, error) {
switch paramType {
switch strings.ToLower(paramType) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test case for this? util_test.go:371 should be the test cases to add to?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have something in mind that's more than what I already added there?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...
{"number", schema.NumberType},
{"NUMBER", schema.NumberType},
{"nil", schema.NullType},
...

Add that to line 385ish in pkg/broker/util_test.go

@shawn-hurley
Copy link
Contributor

@mhrivnak I am re-running the Gate test after the apb base fix that we merged yesterday.

Copy link
Contributor

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently, I cant read. sorry @mhrivnak

@shawn-hurley shawn-hurley merged commit 22e3f36 into openshift:master Dec 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type fields in APB specs are case-sensitive
6 participants