-
Notifications
You must be signed in to change notification settings - Fork 39.2k
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
OpenAPI / Swagger2 spec generation #30233
Conversation
// fixOperation tries to rename operation IDs to make OpenAPI spec valid. The best solution to repeated | ||
// ops is to fix it in the source. If that is not possible, the opFix map would let customize the renaming. If the ID | ||
// is not in the opFix map, the name will be concatenated with a string computed from the path. see inferNamePartFromPath | ||
// for more information. |
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.
How do we fix generation so this is not necessary?
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 should be done in api_installer. We should pass unique Operation name for each operation. There were just too many of them. I think it is better to have swagger 2.0 working and mark it as alpha, then fix those.
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.
Pass this function in to the installer as a fixup func so the implementation in this package knows nothing about Kube.
ded91bb
to
c661b56
Compare
func (o *openAPI) buildModels() (definitions spec.Definitions, err error) { | ||
definitions = spec.Definitions{} | ||
for _, decl := range swagger.NewSwaggerBuilder(*o.config.SwaggerConfig).ProduceAllDeclarations() { | ||
for _, oldNamedModel := range decl.Models.List { |
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.
Why calling it oldNamedModel?
Looks good, mostly naming comments. Would be good to add unit tests for the new code. kubernetes/pkg/master/master_test.go Line 435 in dfcb649
We can add a simple test for swagger.json path. |
Would be great if we can have unit tests for all methods with non-trivial code. |
97b3f57
to
61b69af
Compare
I've added some test that covers most of the code. This is ready for another round of review. |
@@ -272,6 +272,7 @@ func Run(s *options.APIServer) error { | |||
genericConfig.ProxyDialer = proxyDialerFn | |||
genericConfig.ProxyTLSClientConfig = proxyTLSClientConfig | |||
genericConfig.Serializer = api.Codecs | |||
genericConfig.OpenAPIInfo.Title = "Kubernetes" |
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.
Do you also have to set version here?
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.
newConfig set both Title and Version to "Generic API Server" and "unversioned". here we just overwrite the title.
Has verification failures |
Thanks, LGTM. |
1 similar comment
GCE e2e build/test passed for commit c5f1d63. |
Automatic merge from submit-queue |
@@ -1,6 +1,6 @@ | |||
{ | |||
"ImportPath": "k8s.io/kubernetes", | |||
"GoVersion": "go1.6", | |||
"GoVersion": "go1.7", |
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.
Was this change intentional? If not please revert.
cc @wojtek-t
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.
no it was not. sent the PR to revert it.
Now that we have a separate param, we should mention in the release note that admins can turn off this new endpoint by setting EnableOpenAPISupport = false. However for admins to be able to turn it off, we will need to expose it as a command line flag in server_run_options and use that to set config.EnableOpenAPISupport. |
Is there a downside to this endpoint being exposed? On Tue, Aug 23, 2016 at 5:27 PM, Nikhil Jindal notifications@github.com
|
@nikhiljindal, @smarterclayton Sorry I missed these comment. I see swagger is enabled by default and we do not have a server config for it. I don't see any downside having this enabled by default, but I can add server config if you think that is useful. |
I am fine with keeping this enabled by default - should probably mention that explicitly in release notes. Right now it says "Alpha support". We use alpha at other places to mean disabled by default. Re: allowing admins to disable it. Its generally good to allow admins to turn off new features. Not critical. |
Sure, I will send another PR to allow admins to disable it after my improvement #31468 got merged. Do we still have time to change release note of 1.4? |
This is alpha version of OpenAPI spec generation. Generated "/swagger.json" file (accessible on api server) is a valid OpenAPI spec with some warnings that will be fixed in next versions of spec generation. Currently it is possible to generate a client using this spec though I did not test the clients.
reference: #13414
Release note:
This change is![Reviewable](https://camo.githubusercontent.com/2d899f4291d07d3cd2fa4aaae1e3b243f164c23fce87d30a589ace0d496a444c/68747470733a2f2f72657669657761626c652e6b756265726e657465732e696f2f7265766965775f627574746f6e2e737667)