Swagger specs #77
Comments
|
Also: https://github.com/docker-php/docker-php/blob/1.21/docker-swagger.json I agree it would be good to either generate the go types from the spec, or generate the spec from the go code. Both can work, and we seem to have both sources already. I think engine-api is missing a big part of the interface specification (the path, query params, headers, and response codes). Using swagger would force us to include all of those (which I think is good). |
|
Good idea to get it standardized format. Does swagger have something to handle the stream hijacking that is done for attach, etc? |
|
Well, swagger supports plugins/extensions. If there is no way of specifying hijacking, docker can specify hijack mode and code generators would implement it generically. More generally, people don't use hijack/attach very often in the client libraries. I tend to believe it's not much of a common task. They often do container/image managament as expected. |
I've been thinking about this. I think the way to support it would be to use |
we already have the types, let's generate the spec from them. That's what kubernetes does, bth. We might be able to reuse their scripts. |
|
Could also use this spec for auto testing, where the example part of the path can be the acceptance test. |
|
Hey guys, just wanted to chime in. I'm Loc with VMware, working on project VIC. I'm not sure how far you guys have gotten through your swagger efforts, but we've been working on our own version for VIC. @casualjim and I have been working closely to build a docker API server based on swagger. Our swagger spec is based on the docker-php one, but it's been cleaned up a lot. That original spec didn't really work for us. We've also been fixing and adding features to go-swagger to support the docker swagger spec. Our original plan was to hand over the spec to you guys, but it appears you guys are already looking at it. If you guys are interested in this spec, it might accelerate your work. There still quite a bit of work on both the spec and go-swagger to make it fully compliant with the docker api. We should talk. We're literally right around the corner from you guys. |
|
I promise to show up if you have a meeting in SF. |
I'm really interested for knowing why it wasn't working for you ? :) |
|
We're using go-swagger for our generator. Ideally, it should be the same as using swagger, but what I found was there were a lot of mistakes in the original docker-php spec. We got a cleaned up version of the spec from @casualjim, then through trial and error, we cleaned up the spec again to make it work. Our desire was to make sure the server generated with the spec would work flawlessly with both CURL and the docker CLI. That original spec (at least the version I got originally) won't work. It's missing a lot of mime-types, incorrect definitions, missing definitions, etc. Also, @casualjim has had to add features to go-swagger to make the server compliant with docker. We've spent quite a bit of time debugging HTTP headers. In case you're wondering, go-swagger generates golang code. That's why we're using it instead of regular swagger. |
|
Ok, i'm the one who write the swagger spec for docker-php, and i'm really interested to have a shared spec that works for multiple projects (this will, IMO, decrease maintenance for everyone). Don't know if that's ok with you to have a shared spec (and don't know if it's the correct topic to speak about that) feel free to open an issue on https://github.com/docker-php/docker-php if you are interested. |
|
I agree on a shared spec. Our original intent was to get this working and hand it over to Docker and see if they would be interested in maintaining it since they control their APIs. BTW, I want to emphasize that work must also be done on the generator to support functionality required by the docker API server. There's no way to know what this work entails until you run the swagger generated server, run curl request and docker CLI commands and look at the headers. |
|
btw, there's another reason we want to give Docker this spec. We're very interested in this separation of the engine-api server work. It appears both we and Docker are doing similar things but starting in different points. We're trying to build a compliant api server and you guys already have this. We started out with swagger and you guys want to move in this direction. There are some gotchas using swagger. You might have to rethink how the swagger server hooks into the rest of docker, specifically your backend interfaces. We could probably work together on this. |
@joelwurtz, @sflxn please, do not hesitate to open pull request with a spec, completed or not. All contributions are very welcome. I'm pretty sure, nobody else is working on it. That's a much much better starting point than people waiting around 😸 |
|
@joelwurtz some of the issues I fixed through sending a PR. The cases are missing parameters, consumes, produces and so on. One of the funnier ones was: there were some response status codes which are 4O4 instead of 404 so the letter O instead of the number 0. |
|
+1 |
|
@joelwurtz Would you have any qualms if I do a PR today in this repo? Once that happens, I can do a PR on your repo. I sense you want docker-php to be the source of sharing, but I believe keeping the spec in this repo is good place for people to come and collaborate on it. Eventually, this spec and any other that are shared should probably be submitted to swaggerhub. I want to comment about how this spec should be used. For the time being, it's a good source to generate API clients. Generating a server will be much more involved. The docker API server have some custom behaviors that are different from the default behavior of a generated swagger server. @casualjim and I have worked on both the spec and go-swagger to make it come as close as we can, but there are still differences. If there is a desire to move to a swagger based server implementation, I recommend a staged approach. First, refactor data structures passed into the backend methods, adding swagger annotation in the process. The refactoring of these arguments need to be disentagled from docker specific internal structures. An even better approach would be to generate swagger models from the spec and refactor the backends to use those data structures. Down the road, it will become easier to migrate over to a swagger based server. |
Go ahead and no, i don't want docker-php to be the source of sharing on the contrary i would prefer to have a dependency on this repo to reduce my maintenance cost.
I think there is 2 approach :
IMO, the second approach is the best regarding the current status on docker (no bc break), and in this view proposing a swagger spec to this repo is more for testing, so this will ensure that the generation of the spec is correct about the api vision. |
|
Joel, I agree that annotation is a great way to approach when there is an existing code base. There is some value using a starting spec to generate models and using those swagger generated data structures as a guide for updating the existing code. For example, the current server routes can easily be swapped out, in favor of a swagger based server. The swagger generated server is really nothing more than a REST server front end. The data structure generated (models) from the swagger spec can then be passed into the backends or they can be massaged into the existing data structures passed into the backends. |
|
@casualjim can chime in here re: go-swagger |
|
the least risky path is to use the documentation comments. Later on perhaps it can use the generated server but the main thing is that it gets a swagger spec that is generated from the actual code. |
|
I agree with @casualjim, annotation is less risky right now. There are still more work that needs to go into go-swagger to be able to handle all the unique cases that the docker server currently handles. We can work on beefing up go-swagger on our end to get it there, but it would be tough to switch over the rest handling to swagger on the server side right now. |
|
I have been experimenting with adding Swagger annotation to the source code: moby/moby@master...bfirsh:generate-swagger-specification This will allow us to automatically generate the sort of thing in #168 and, helpfully, also forces to add loads of documentation to the API. I'm running into a few cases where Swagger can't generate the correct docs from the objects we have, so there's going to be a bit of refactoring needed to make that work. Refactoring for the better though - it forces us to have clean objects for representing API requests and responses, etc. There are also a few things that go-swagger can't really represent properly (e.g. JSON responses for success, plain text for error). Perhaps we should actually fix these problems in the API rather than try and shoehorn Swagger into the problems. ;) |
|
We have had complaints about the plain text error responses being unexpected. |
|
Any progress on this? From what I understand this would allow us to generate libraries for other languages using codegen. |
|
Just updated our swagger schema with last API Changes : https://github.com/docker-php/docker-php/blob/1.24/docker-swagger.json And IMO, this is really needed, i can't say the number of times where i have see errors on the actual documentation (fields not displayed, wrong field names, updates not reported, a lot of missing informations). I know i can do pull requests to fix all of this, but i'm afraid this will continue to have errors as the api evolves due to the current way of working. Having an auto generated documentation will really help to also generate a synchronized and better documentation than the current one (and allow code generation, but for me this is not the primary goal) Given this, @bfirsh is there any way to help ? I'm not a go expert, but i can take time to update my knowledge and advance this, unless this is not a wanted feature ? |
|
This is definitely wanted and I'm currently cranking through the whole API adding go-swagger definitions. See my branches above for work-in-progress as of a few days ago. Unfortunately I'm running into lots of go-swagger issues, but the author is pretty responsive and helpful. Once I've added a first pass of definitions, I'd really appreciate some help checking it over to make sure it is accurate. Once we've got that in, then we can generate some API documentation. 😄 BTW – thanks for your work on the manually created version. I've been using it as a reference for the auto-generated one to make sure it is correct! |
|
Current work in progress, automatically generated from the branches above: You can plug it into http://petstore.swagger.io/ and click "explore" to get a GUI. Lots of stuff missing/broken still, but hopefully gives you an idea of where it's heading. |
|
Anybody have any good ideas on how to test this? I was thinking some kind of sanity test to generate it and check some expected stuff is in it, but it would be quite nice to check it's actually correct somehow. I can imagine a fully fledged integration suite which actually tries to generate a client out of the swagger.json, then tests that against a running version of the code it was generated it from. Sounds a bit ambitious/slow for a first pass though... |
|
Swagger spec is a json schema, you can test that your spec is valid with this schema : https://github.com/OAI/OpenAPI-Specification/blob/master/schemas/v2.0/schema.json Not sure if it handles all the contraints, but it's a good start |
|
the most complete working validator that I know of for a swagger spec is a nodejs tool. |
|
@casualjim Ah, hello! Didn't connect you in this thread with you on go-swagger. :D Checking the validation of the spec is a good idea. I wonder if there are some simple smoke tests we can do to check it's actually correct, though. My fear is that somebody will change something in the API, then forget to change the annotations, and nobody will notice because it won't break a test. |
I don't think it's possible to have such tests unless docker itself use the swagger schema to generate its client (so this will be tested by the process of writing). I know elasticsearch has some kind of specification tests that are used by many clients for integration: https://github.com/elastic/elasticsearch/tree/master/rest-api-spec/src/main/resources/rest-api-spec/test which we can mimic, but, sometimes, they are still inconsistencies (i got this recently in here : elastic/elasticsearch-php@bd00e22#diff-aefcee0e9e5740f9f538f702a848a6c1L26) |
|
I'm having real trouble generating a spec from annotations on the existing source code, mostly running into issues with go-swagger and go-swagger not generating quite the specs that I want. I'm also spending a lot of time refactoring code to make it similar to what the go-swagger code generator would produce anyway (e.g. proper parameters, responses, models, etc). Which is making me think... why don't we just do code gen instead? #168 is a great place to start for our Swagger spec, then we can use that to generate a set of parameters, responses, and models. In theory it'll generate the same objects as we need to have in the source code anyway, so all of the existing server and client code will more-or-less work. I'll experiment with generating code from that spec and how well it works... |
|
Okay I got a bit overly enthusiastic there. Code generation looks like quite dauntingly large piece of work. 😓 Stepping back a bit, I think setting a goal might help focus the work. Let's say we want to replace the API documentation with something automatically generated from Swagger, ala moby/moby#8402. That'll give us much better API documentation, and as a nice side-effect give us a good swagger.json. Given this goal, I think the easiest way to achieve it is just to manually write a swagger.json file. Generating the swagger.json from comments in the source code is hard to debug, forces us to write our source code how go-swagger expects to read it, and just generally a bit frustrating. If adding Swagger annotations has been a PITA for me, then it's bound to be a PITA for other maintainers. Above all – this is a logical first step towards automatically generating types from swagger.json. Annotating source code that might be thrown away eventually is not. A counter-argument could be that the Swagger definition might get forgotten if it's not in the source code. But, if the manually written file is source of truth for the API documentation, that will force us to update it when the API updates. So – I'm going to just try and make some really good API documentation, using #168 as a starting point. |
|
I think that's a good plan. In the long run I think generating code from the spec is the right way to do it. It may feel a bit uncomfortable at first, especially if you aren't used to the idea, but I think making the spec the source of truth focuses on API design first, which leads to a better design. |
|
Here is my work in progress, for those interested: https://bfirsh.github.io/docker-api-reference/ It's obviously unfinished. At the moment I'm just ploughing through it porting the existing API docs to a Swagger spec. Turns out a lot of the API docs are incomplete/wrong/poorly written/inconsistent, so I'm also using this opportunity to tidy that up! I would be interested in all your feedback. Early and often, and all that. |
|
Nice ! I'm currently in holidays but when i came i'm very motivated to work on this, will certainly do many pull requests with the changes that i have done over the past months on docker-php |
|
+1 really nice. At first was a little put-off by having to update the swagger spec on API changes... but it's hella better than dealing with the markdown API spec we currently have... which is pretty much always out of date and difficult to navigate. |
|
I've opened moby/moby#27117 to start generating types from the spec. |
It came up a lot in the Docker Remote API rework discussions: moby/moby#15188 moby/moby#5893 I also think swagger is a great way to describe APIs.
If there's a way maybe we can declare types/* in swagger specs and generate source files using swagger code generators, that would be very preferable as other language SDKs can take advantage of the swagger specs as well.
Just leaving this here.
The text was updated successfully, but these errors were encountered: