Skip to content

Replace go-swagger with swagger-gen#36714

Closed
dnephin wants to merge 4 commits intomoby:masterfrom
dnephin:use-swagger-gen
Closed

Replace go-swagger with swagger-gen#36714
dnephin wants to merge 4 commits intomoby:masterfrom
dnephin:use-swagger-gen

Conversation

@dnephin
Copy link
Member

@dnephin dnephin commented Mar 27, 2018

This PR replaces the tool used for api/types code generate. We have been using go-swagger but this has not worked well:

  • the generation includes an iteration over a map. When the order changes the build will fail when it tries to validate the vendoring of the generated code was done correctly.
  • the above bug can't really be fixed because go-swagger removed support for generating only types.
  • go-swagger used templates templates to generate code, which proved to be difficult to work with, and it never gave us full control over the templates.
  • the go-swagger defaults required us to add a lot of x-nullable: false all over the spec and often ignored Title when it was set.

The new tool is currently at https://github.com/dnephin/swagger-gen-types, but I would like to move it to the moby organization on github, since it's primarily only going to be used with moby/moby.

The rules for generating types are simple, and the code is only ~300 lines. It uses the https://github.com/go-openapi for parsing the swagger spec, and https://godoc.org/github.com/dave/jennifer/jen for code generation.

This new tool should make it a lot easier to convert more types to code generation, and makes it a lot easier to customize the code that it generated.

dnephin added 3 commits March 27, 2018 14:51
Signed-off-by: Daniel Nephin <[email protected]>
Signed-off-by: Daniel Nephin <[email protected]>
@dnephin
Copy link
Member Author

dnephin commented Mar 27, 2018

It looks like yamllint was removed as part of the multi-stage Dockerfile PR, but validation never run because our validation checks for changes before running.

I'll look at restoring yamllint.

@thaJeztah thaJeztah changed the title Repalce go-swagger with swagger-gen Replace go-swagger with swagger-gen Apr 5, 2018
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Thanks! Left some thoughts 👍

".*\\.pb\\.go",
"dockerversion/version_autogen.go",
"api/types/container/container_.*",
"api/types/.*-generated\.go",
Copy link
Member

Choose a reason for hiding this comment

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

Should this have double backslashes?


The `Status` field is optional, and is omitted if the volume driver
does not support this feature.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Not a big fan of the block-style comments; seems they're generated by this https://github.com/dave/jennifer/blob/master/jen/comments.go#L85-L93, so not sure there's an easy workaround (other than splitting on newlines and appending a single-line comment for each line)

Copy link

Choose a reason for hiding this comment

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

Yeah if there's a newline in a comment, jennifer will use multi-line syntax. However, if you prefix your comment with // it'll be rendered as-is. You'd need to manually add the // to the start of every line though. Feel free to ping me if you'd like any more jennifer help!

}

// OK response to ContainerWait operation
type ContainerWaitResponse struct {
Copy link
Member

Choose a reason for hiding this comment

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

Wondering what determines if this is named ContainerWaitOKResponse vs ContainerWaitResponse.

Although I like "response"; "body" may be more factual (i.e. it's not the full response); do you know where the code is to generate the names?

Basically; should we try to make the names match the old names?

Copy link
Member

Choose a reason for hiding this comment

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

Comment doesn't look to be correct godoc as well

cd "$GOPATH/src/github.com/dnephin/swagger-gen"
git checkout -q "$SWAGGER_GEN_COMMIT"

go build -o ${PREFIX}/swagger-gen .
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: need to replace spaces with a tab

COPY --from=swagger /usr/local/bin/swagger* /usr/local/bin/
COPY --from=frozen-images /docker-frozen-images /docker-frozen-images
COPY --from=gometalinter /opt/gometalinter/ /usr/local/bin/
COPY --from=swaggergen /opt/swaggergen/ /usr/local/bin/
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this hunk belong to the first commit in the series?

@olljanat
Copy link
Contributor

@dnephin this one needs to be rebased

@derek derek bot added the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 22, 2018
@cpuguy83
Copy link
Member

This hasn't been touched in a long time and no one seems to be taking it up, so I'm going to go ahead and close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/api API status/failing-ci Indicates that the PR in its current state fails the test suite status/2-code-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants