-
Notifications
You must be signed in to change notification settings - Fork 284
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
API v2 - specify API version via headers or in url #3431
Conversation
It would be nice to have some tests for this with |
req.accept =~ /version=([\d\.]+)/ | ||
if (version = $1) # version is specified in header | ||
header_match = req.accept.match(/version=([\d\.]+)/) | ||
route_match = req.fullpath.match(/api\/v\d\//) |
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'm wondering if %r{api/v\d/}
would be easier to read here?
ACK pending comments and Jenkins. Thanks again for working on this. |
Opened issue http://projects.theforeman.org/issues/3754 for the tests ACK pending Jenkins. |
@@ -18,8 +18,13 @@ def initialize(options) | |||
end | |||
|
|||
def matches?(req) | |||
req.accept =~ /version=([\d\.]+)/ | |||
if (version = $1) # version is specified in header | |||
header_match = req.accept.match(/version=([\d\.]+)/) |
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.
It looks like request.accept
could return nil. If you update this to the following, I think it should fix the breaking test:
header_match = req.accept.try(:match, /version=([\d\.]+)/)
Katello::Api::V1::UsersControllerTest.test_list_owners_username
closing due to merge of PR continued in #3461. |
No description provided.