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

Support creating a Swagger def with no schemes #1075

Merged
merged 1 commit into from
Nov 4, 2019
Merged

Support creating a Swagger def with no schemes #1075

merged 1 commit into from
Nov 4, 2019

Conversation

ailurarctos
Copy link
Contributor

Make it so if you do not specify the schemes then no schemes
will be present in the generated Swagger definition. The
OpenAPIv2 spec says this means to use the same scheme that
was used to access the Swagger definition itself.

Fixes #1069

Make it so if you do not specify the schemes then no schemes
will be present in the generated Swagger definition. The
OpenAPIv2 spec says this means to use the same scheme that
was used to access the Swagger definition itself.

Fixes #1069
@codecov-io
Copy link

Codecov Report

Merging #1075 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1075      +/-   ##
=========================================
- Coverage   54.01%     54%   -0.02%     
=========================================
  Files          42      42              
  Lines        4199    4198       -1     
=========================================
- Hits         2268    2267       -1     
  Misses       1683    1683              
  Partials      248     248
Impacted Files Coverage Δ
protoc-gen-swagger/genswagger/types.go 0% <ø> (ø) ⬆️
protoc-gen-swagger/genswagger/template.go 57.67% <ø> (-0.04%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a54d88...8b4befd. Read the comment docs.

@achew22
Copy link
Collaborator

achew22 commented Nov 4, 2019

I remember from earlier threads that this has the affect that some generators will default to http. Is that something we want to blindly opt people into? What would you think about just defaulting it to []string{"https"} and leaving people to override it if they want something else?

cc @johanbrandhorst I know you have more context on this

@ailurarctos
Copy link
Contributor Author

@achew22 unfortunately if the default is not empty there is currently no way to override the default with an empty value. I mentioned some alternatives to changing the default in #1069 (comment) but after further discussion with @johanbrandhorst it seemed like changing the default might be okay after all.

I remember from earlier threads that this has the affect that some generators will default to http.

Yes, I looked into three code generators in #1069 (comment) and unfortunately go-swagger will change from using HTTPS to HTTP. This change will also cause swagger-codegen to change from HTTP to HTTPS while openapi-generator will continue to use HTTP as it does with the current default.

@johanbrandhorst
Copy link
Collaborator

I think, on balance, it is not our responsibility to limit the functionality of our project in order to avoid potential security shortcomings of other projects. The rules we should be bound by for the swagger generator should be the swagger spec. We'll have to exercise judgment, of course, but I think we can get the best of both worlds by contributing safer defaults to the downstream projects, too. Do you agree @achew22? I'm happy to discuss further, the security of our users is of course paramount.

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

LGTM, @achew22 will wait for your response before merging.

@achew22
Copy link
Collaborator

achew22 commented Nov 4, 2019

If the analysis has been done then I'm comfortable with it.

@ailurarctos thanks for your contribution and for putting up with me 😉!

@achew22 achew22 merged commit 7dddcb1 into grpc-ecosystem:master Nov 4, 2019
@ailurarctos
Copy link
Contributor Author

Thanks very much @johanbrandhorst and @achew22!

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

Successfully merging this pull request may close these issues.

protoc-gen-swagger: support generating a Swagger definition with no schemes
5 participants