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

Don't use Swagger spec path for API URL #295

Merged
merged 1 commit into from
Sep 18, 2018

Conversation

mulmschneider
Copy link
Contributor

According to the Swagger spec we should not use the path part of the
spec URL, but instead just use the host part of the URL (see the
existing comment for build_api_serving_url, as well as
https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#fixed-fields
).

Also update tests to match new behaviour.

I don't know if bravado users depend on the current behavior - feedback appreciated.

According to the Swagger spec we should not use the path part of the
spec URL, but instead just use the host part of the URL (see the
existing comment for build_api_serving_url, as well as
https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#fixed-fields
).

Also update tests to match new behaviour.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.445% when pulling 1083636 on mulmschneider:build_url_no_basepath into 947d7fc on Yelp:master.

@mulmschneider
Copy link
Contributor Author

Not sure why the 'docs' part of the build failed - seems to be unrelated from this PR?

Copy link
Collaborator

@macisamuele macisamuele left a comment

Choose a reason for hiding this comment

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

@mulmschneider thanks a lot for your contribution.

About the docs test failure, don't worry about it has already been addressed by #296

@macisamuele macisamuele merged commit 2946228 into Yelp:master Sep 18, 2018
@DStape
Copy link
Contributor

DStape commented Sep 20, 2018

Hello, I've opened #299 because this change has caused some test failures for us, there's more detail in that thread, any feedback is appreciated, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants