-
Notifications
You must be signed in to change notification settings - Fork 77
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
updated swagger spec for API 1.1.0 #175
Conversation
the previous swagger spec was broken because of a basic code generation. this needs to be maintained more carefully and not reliant upon generated outputs. @akutz fixed this part: You must have a tag for a parameter of type . It was added wherever it was omitted, setting the schema to a variant type instead. It was omitted in a lot of requests as well as one response. Within the lookups, I (@kacole2) changed the lookup query reuirement from FALSE to TRUE. For some reason the code would fail to execute unless it was set to TRUE. There were some memory pointers happening and it will have to be looked at once again. To get a better idea of how this is all generated using go-swagger, checkout the gorackhd repo and follow the directions listed there. https://github.com/emccode/gorackhd Signed-off-by: Kendrick Coleman <kendrickcoleman@gmail.com>
@RackHD/corecommitters please review. |
ok to test |
@@ -1432,7 +1437,7 @@ paths: | |||
- name: q | |||
in: query | |||
description: query object to pass through to waterline. | |||
required: false | |||
required: true |
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.
@benbp can you take a quick look at the code and verify? The relevant line is https://github.com/RackHD/on-http/blob/master/lib/api/1.1/northbound/lookups.js#L31, so the main branch of this definitely wants to see a q
there, but I think the intent was that waterline.lookups.findByTerm(null)
would return all values. If that's correct (and works) then q
isn't required.
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 originally tried to make this work with Q set to FALSE but just couldn't. as soon as i switched it to TRUE, then all of sudden the queries would work. Let me try testing again by throwing in nil
. https://github.com/emccode/docker-machine-rackhd/blob/master/rackhd.go#L155
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 just tested this and here are the results.
When lookups q
query is set to TRUE:
- Pass in q as a string (Node, IP, Mac) and the returned result is the expected array
- Pass in
nil
, then and error comes back withcannot use nil as type string in field value
- Pass in
""
(completely blank), the returned result is everything in an array as expected
When lookups q
query is set to FALSE:
- Pass in q as a string (Node, IP, Mac) and the returned result is the error:
cannot use "56dde3441722c192796e3a38" (type string) as type *string in field value
- Pass in
nil
, the returned result is everything in an array as expected
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 on-http 1.1 API implementation doesn't use swagger in any capacity. The source of that validation error message is not RackHD.
When the python library is updated to use this change (occurs post commit), this should break our functional tests here: https://github.com/RackHD/RackHD/blob/master/test/tests/api/v1_1/lookups_tests.py#L51.
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.
@zyoung51 comment ^^ still holds true when required is set to true. The python functional tests run tests without the 'q' parameter, both library and functional tests will need to be updated if we force 'q' parameter.
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.
@jlongever The tests and library are correct now. We shouldn't be setting the required flag for an optional parameter even if it's not actually enforced by the 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.
fwiw, we're using swagger-codegen and haven't had issues with the optional 'q' parameter.
Not sure about the required |
|
test this please |
@brianparry i have my manual testing notes in the thread. unless that wasn't directed at me. |
@kacole2 Brian's message is a trigger phrase for Jenkins and is only meant for that system. |
@kacole2 that's the phrase that commiters can invoke to have jenkins do a test build/run against the code - not meant for you specifically |
|
Sorry for the confusion @kacole2. That comment was for Jenkins not you :) |
Jenkins, test this please |
Mr Jenkins bot, test this please |
@RackHD/corecommitters looks like earlier testing hung or something, trying to retrigger |
bah, seen this with other checks. The status gets stuck on running. I've had to close/reopen the PR before to "dislodge" it. |
will do that, thanks |
|
test this please |
|
test this please |
1 similar comment
test this please |
@kacole2 @brianparry @jlongever is this PR still valid? it's pending for a long time. not sure if it's valid after two months' many codes update, and conflicts need to be resolved. |
@anhou close it. will re-open if this isn't fixed in v2.0 API. The gorackhd library is going to keep a manual swagger-spec with this changed to keep it stable and working as expected |
the previous swagger spec was broken because of a basic code generation. this needs to be maintained more carefully and not reliant upon generated outputs. @akutz fixed this part: You must have a tag for a parameter of type . It was added wherever it was omitted, setting the schema to a variant type instead. It was omitted in a lot of requests as well as one response. Within the lookups, I (@kacole2) changed the lookup query reuirement from FALSE to TRUE. For some reason the code would fail to execute unless it was set to TRUE. There were some memory pointers happening and it will have to be looked at once again. To get a better idea of how this is all generated using go-swagger, checkout the gorackhd repo and follow the directions listed there. https://github.com/emccode/gorackhd
Signed-off-by: Kendrick Coleman kendrickcoleman@gmail.com