Automatic API generation via go-restful by bgrant0607 · Pull Request #2282 · kubernetes/kubernetes

bgrant0607 · 2014-11-11T07:33:17Z

Partially addresses #1052.

There's lots more work left to do here, but I got this working, so I thought I'd share it.

go-restful provides a more structured handler registration mechanism, with the ability to document a number of aspects of the API. The object definitions are extracted via reflection and their descriptions are extracted from field tags (added by the author of go-restful upon my request). It uses https://github.com/swagger-api/swagger-ui for the doc UI, and also provides a discovery API that can be used to query available endpoints and json schemas.

In order for the initial version of this to be merged, two main things need to be done:

Fix apiserver_test and make all the tests pass
Update and merge the PR that adds go-restful to Godeps

Beyond that, I'd like to iterate on cleaning this up and adding documentation in other PRs:

Customize the swagger UI for our needs
Figure out how to copy https://github.com/swagger-api/swagger-ui/dist to the master
Figure out what to do with the custom verbs (watch, proxy, redirect)
Convert all handlers to native go-restful style
Convert handler parameters to go-restful parameters
Perhaps split REST handlers into per-verb handlers
Add descriptions to all handlers
Add description tags to all types.go object fields
Add documentation of other details (return codes, input/output mime types, etc.)
Fix go-restful bugs

/cc @lavalamp @smarterclayton @erictune

smarterclayton · 2014-11-11T16:02:59Z

Flexibility, no other reason.

Took away that flexibility for now. We can add it back later if needed.

bgrant0607 · 2014-11-12T08:31:16Z

Will rebase and squash once #2222 is merged.

lavalamp · 2014-11-13T01:19:00Z

please use path.Join

bgrant0607 · 2014-11-13T01:44:07Z

Rebase is now pushed. PTAL.

lavalamp · 2014-11-13T01:53:27Z

You can condense the above with .Infof("InstallREST: %v kinds: %#v", version, kinds)

lavalamp · 2014-11-13T02:24:55Z

missing gofmt. I'd like to give this another look-through before it's merged, I'm still digesting that gigantic function...

bgrant0607 · 2014-11-13T02:44:20Z

Feedback addressed.

smarterclayton · 2014-11-13T02:49:47Z

Same here - promise to look in morning.

On Nov 12, 2014, at 9:25 PM, Daniel Smith notifications@github.com wrote:

missing gofmt. I'd like to give this another look-through before it's merged, I'm still digesting that gigantic function...

—
Reply to this email directly or view it on GitHub.

bgrant0607 · 2014-11-13T05:11:04Z

verify-gofmt and e2e pass in my sandbox. Not sure what's up with Travis.

erictune · 2014-11-13T05:35:37Z

It looks like Consume takes as many mime types as you want. So maybe enumerate the ones we take rather than accept all?

Now, the question is what mime type(s) YAML is.
I think ws.Consumes(restful.MIME_JSON, "application/x-yaml", "text/yaml") should do it

Yes, Consume takes an arbitrary number of types. However, not all clients set a content type at all, and there is no standard yaml type. I figure this can be fixed in a follow-up PR, after we get our client library and tools squared away.

erictune · 2014-11-13T05:39:30Z

Didn't do a super detailed review, but looks good overall.

You can git rid of the whoami handler if it is causing you trouble. I haven't needed it like I thought I would.

bgrant0607 · 2014-11-13T06:01:23Z

Thanks @erictune. I know there's lots more to do, but I'm just trying to get in the minimal viable change before other PRs break it again. It will be easier to iterate on it once it's merged.

bgrant0607 · 2014-11-13T06:05:36Z

Could I be running a different version of gofmt than Travis? Travis complains:

!!! gofmt needs to be run on the following files: 
./pkg/client/containerinfo_test.go
./pkg/apiserver/apiserver.go

I ran gofmt on these 2 files and pushed them because verify-gofmt complained on my system. However, how Travis complains about them. gofmt doesn't have a --version flag. The change to the first file looks like:

--- a/pkg/client/containerinfo_test.go
+++ b/pkg/client/containerinfo_test.go
@@ -114,7 +114,7 @@ func TestHTTPContainerInfoGetterGetContainerInfoSuccessfully(t *testing.T) {
        }
        cinfo := itest.GenerateRandomContainerInfo(
                "dockerIDWhichWillNotBeChecked", // docker ID
-               2, // Number of cores
+               2,                               // Number of cores
                req,
                1*time.Second,
        )
@@ -127,7 +127,7 @@ func TestHTTPContainerInfoGetterGetRootInfoSuccessfully(t *testing.T) {
        }
        cinfo := itest.GenerateRandomContainerInfo(
                "dockerIDWhichWillNotBeChecked", // docker ID
-               2, // Number of cores
+               2,                               // Number of cores
                req,
                1*time.Second,
        )
@@ -140,7 +140,7 @@ func TestHTTPContainerInfoGetterGetContainerInfoWithError(t *testing.T) {
        }
        cinfo := itest.GenerateRandomContainerInfo(
                "dockerIDWhichWillNotBeChecked", // docker ID
-               2, // Number of cores
+               2,                               // Number of cores
                req,
                1*time.Second,
        )
@@ -153,7 +153,7 @@ func TestHTTPContainerInfoGetterGetRootInfoWithError(t *testing.T) {
        }
        cinfo := itest.GenerateRandomContainerInfo(
                "dockerIDWhichWillNotBeChecked", // docker ID
-               2, // Number of cores
+               2,                               // Number of cores
                req,
                1*time.Second,
        )

and to the second:

--- a/test/integration/auth_test.go
+++ b/test/integration/auth_test.go
@@ -321,7 +321,7 @@ func getTestRequests() []struct {
                {"DELETE", "/api/v1beta1/events/a" + syncFlags, "", code200},

                // Normal methods on bindings
-               {"GET", "/api/v1beta1/bindings", "", code405},            // Bindings are write-only
+               {"GET", "/api/v1beta1/bindings", "", code405},     // Bindings are write-only
                {"POST", "/api/v1beta1/pods" + syncFlags, aPod, code200}, // Need a pod to bind or you get a 404
                {"POST", "/api/v1beta1/bindings" + syncFlags, aBinding, code200},
                {"PUT", "/api/v1beta1/bindings/a" + syncFlags, aBinding, code500}, // See #2114 about why 500

erictune · 2014-11-13T06:10:07Z

Did you forget that you have to stage files again after running gofmt?
Sometimes I forget this. Check if you have uncommitted changes to those
files.

On Wed, Nov 12, 2014 at 10:05 PM, bgrant0607 notifications@github.com
wrote:

Could I be running a different version of gofmt than Travis? Travis
complains:

!!! gofmt needs to be run on the following files:
./pkg/client/containerinfo_test.go
./pkg/apiserver/apiserver.go

I ran gofmt on these 2 files and pushed them because verify-gofmt
complained on my system. However, how Travis complains about them. gofmt
doesn't have a --version flag. The change to the first file looks like:

--- a/pkg/client/containerinfo_test.go
+++ b/pkg/client/containerinfo_test.go
@@ -114,7 +114,7 @@ func TestHTTPContainerInfoGetterGetContainerInfoSuccessfully(t *testing.T) {
}
cinfo := itest.GenerateRandomContainerInfo(
"dockerIDWhichWillNotBeChecked", // docker ID
          2, // Number of cores
          2,                               // Number of cores
        req,
        1*time.Second,
)
@@ -127,7 +127,7 @@ func TestHTTPContainerInfoGetterGetRootInfoSuccessfully(t *testing.T) {
}
cinfo := itest.GenerateRandomContainerInfo(
"dockerIDWhichWillNotBeChecked", // docker ID
          2, // Number of cores
          2,                               // Number of cores
        req,
        1*time.Second,
)
@@ -140,7 +140,7 @@ func TestHTTPContainerInfoGetterGetContainerInfoWithError(t *testing.T) {
}
cinfo := itest.GenerateRandomContainerInfo(
"dockerIDWhichWillNotBeChecked", // docker ID
          2, // Number of cores
          2,                               // Number of cores
        req,
        1*time.Second,
)
@@ -153,7 +153,7 @@ func TestHTTPContainerInfoGetterGetRootInfoWithError(t *testing.T) {
}
cinfo := itest.GenerateRandomContainerInfo(
"dockerIDWhichWillNotBeChecked", // docker ID
          2, // Number of cores
          2,                               // Number of cores
        req,
        1*time.Second,
)
and to the second:

--- a/test/integration/auth_test.go
+++ b/test/integration/auth_test.go
@@ -321,7 +321,7 @@ func getTestRequests() []struct {
{"DELETE", "/api/v1beta1/events/a" + syncFlags, "", code200},
            // Normal methods on bindings
          {"GET", "/api/v1beta1/bindings", "", code405},            // Bindings are write-only
          {"GET", "/api/v1beta1/bindings", "", code405},     // Bindings are write-only
        {"POST", "/api/v1beta1/pods" + syncFlags, aPod, code200}, // Need a pod to bind or you get a 404
        {"POST", "/api/v1beta1/bindings" + syncFlags, aBinding, code200},
        {"PUT", "/api/v1beta1/bindings/a" + syncFlags, aBinding, code500}, // See #2114 about why 500
—
Reply to this email directly or view it on GitHub
#2282 (comment)
.

bgrant0607 · 2014-11-13T06:17:19Z

I have no unpushed changes. If you look at the commit history, it's the pushes of gofmt changes that caused the Travis failures.

erictune · 2014-11-13T06:23:14Z

For the container_info_test.go, try introducing a newline above and below
the offending line to disable gofmt from aligning it.

For apiserver.go, you pasted the wrong thing in your mail, but maybe the
same thing will work.

On Wed, Nov 12, 2014 at 10:17 PM, bgrant0607 notifications@github.com
wrote:

I have no unpushed changes. If you look at the commit history, it's the
pushes of gofmt changes that caused the Travis failures.

—
Reply to this email directly or view it on GitHub
#2282 (comment)
.

smarterclayton · 2014-11-13T16:15:25Z

But if they're unversioned we can't evolve them without introducing new fields and deprecating old ones. I'm somewhat scared about introducing these until we have a good idea what they should be doing, because we can't change them in a version compatible way. Do you have confidence these are the right objects?

We already return APIVersions. This just moves it.

APIResources could be versioned, I suppose.

Commented out APIResources and added TODOs for now.

smarterclayton · 2014-11-13T16:22:32Z

Having to do this seems to defeat the purpose of taking apiserver.Mux.

Thanks. Fixed.

bgrant0607 · 2014-11-13T17:17:50Z

I did have an incompatible version of gofmt in my path. Now using the gofmt from go 1.3.

bgrant0607 · 2014-11-13T19:05:38Z

Travis is green. All feedback addressed.

mindscratch · 2014-11-13T19:06:32Z

just want to throw in my +1

smarterclayton · 2014-11-13T20:02:26Z

Can you update your commit message to not be WIP and remove the error? Seems like it's busted

lavalamp · 2014-11-13T20:06:15Z

gofmt: it did change at some point; I forget what version we standardized on. It follows your go version.

I have version 1.3.

lavalamp · 2014-11-13T20:47:48Z

From a readability standpoint, I think it would be great if the body of this loop were moved into its own function, e.g., installSingleHandler or some such. I'm ok merging without this if it will get cleaned up soon, although I'm feeling like in general promised cleanup has not been happening at the promised rate and I'm getting reluctant to say things like this...

Maybe we need a cleanup week.

On Nov 13, 2014, at 3:48 PM, Daniel Smith notifications@github.com wrote:

In pkg/apiserver/apiserver.go:

// create one for the tests, also

// Success:

// http.StatusOK, http.StatusCreated, http.StatusAccepted, http.StatusNoContent

//

// test/integration/auth_test.go is currently the most comprehensive status code test

// TODO: eliminate all the restful wrappers

// TODO: create a separate handler per verb

h := func(req *restful.Request, resp *restful.Response) {

glog.V(3).Infof("User-Agent: %s\n", req.HeaderParameter("User-Agent"))

strippedHandler.ServeHTTP(resp.ResponseWriter, req.Request)

}

// TODO: factor out per-kind code

for path, storage := range g.handler.storage {

glog.V(3).Infof("Installing /%s/%s\n", version, path)
From a readability standpoint, I think it would be great if the body of this loop were moved into its own function, e.g., installSingleHandler or some such. I'm ok merging without this if it will get cleaned up soon, although I'm feeling like in general promised cleanup has not been happening at the promised rate and I'm getting reluctant to say things like this...

—
Reply to this email directly or view it on GitHub.

I'll refactor it now, esp. since it won't be merged until tomorrow, anyway.

Thanks, much appreciated

lavalamp · 2014-11-13T20:51:11Z

LGTM with responses to my last two comments.

bgrant0607 · 2014-11-14T03:43:09Z

Feedback addressed, but now I'm having a really weird gofmt issue. I get a gofmt error that prevents me from rebasing, even though I've removed the old gofmt binary and even if I specify --no-verify AND remove the prepare-commit-msg hooks. Sigh.

bgrant0607 · 2014-11-14T16:51:42Z

Learned enough git to make the rebase work, and reinstalled go on my system for good measure. Ok to merge now?

smarterclayton · 2014-11-14T19:21:52Z

About to merge, rerunning travis in case a fluke occured.

bgrant0607 · 2014-11-14T20:59:27Z

@smarterclayton Travis is green both on head and on this PR. Did you see a failure earlier?

lavalamp · 2014-11-14T21:12:23Z

LGTM, thanks for rearranging.

Automatic API generation via go-restful

smarterclayton · 2014-11-14T21:16:13Z

I'm in the habit of rebuilding Travis right before big refactor merges now because Travis won't rebuild if someone else delivers function. Bit me a few times.

On Nov 14, 2014, at 3:59 PM, bgrant0607 notifications@github.com wrote:

@smarterclayton Travis is green both on head and on this PR. Did you see a failure earlier?

—
Reply to this email directly or view it on GitHub.

…-disabled-older-kubelet-ocp-master OCPBUGS-55632: Fix node expansion on older kubelets

smarterclayton reviewed Nov 11, 2014
View reviewed changes

bgrant0607 mentioned this pull request Nov 12, 2014

Generate API docs from Go structs or vice versa #1052

Closed

bgrant0607 changed the title ~~WIP: Automatic API generation via go-restful~~ Automatic API generation via go-restful Nov 12, 2014

bgrant0607 force-pushed the docgen branch from 8b972ac to 59b3c4a Compare November 12, 2014 23:17

bgrant0607 mentioned this pull request Nov 13, 2014

Create a standalone k8s binary, capable of running a full cluster #2121

Merged

lavalamp reviewed Nov 13, 2014
View reviewed changes

bgrant0607 force-pushed the docgen branch from 59b3c4a to 7219ad9 Compare November 13, 2014 01:43

lavalamp reviewed Nov 13, 2014
View reviewed changes

erictune reviewed Nov 13, 2014
View reviewed changes

smarterclayton reviewed Nov 13, 2014
View reviewed changes

bgrant0607 force-pushed the docgen branch from 2664a9c to 309058a Compare November 13, 2014 19:08

lavalamp reviewed Nov 13, 2014
View reviewed changes

Automatic API generation by adopting go-restful

7583e1a

bgrant0607 force-pushed the docgen branch from eee4963 to 7583e1a Compare November 14, 2014 16:49

lavalamp added a commit that referenced this pull request Nov 14, 2014

Merge pull request #2282 from bgrant0607/docgen

9430bb3

Automatic API generation via go-restful

lavalamp merged commit 9430bb3 into kubernetes:master Nov 14, 2014

smarterclayton mentioned this pull request Jan 4, 2015

High-level guidance about what types of updates to use in what scenario #3153

Closed

bertinatto pushed a commit to bertinatto/kubernetes that referenced this pull request May 7, 2025

Merge pull request kubernetes#2282 from gnufied/fix-expansion-recover…

aa347f0

…-disabled-older-kubelet-ocp-master OCPBUGS-55632: Fix node expansion on older kubelets

Conversation

bgrant0607 commented Nov 11, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgrant0607 commented Nov 12, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgrant0607 commented Nov 13, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lavalamp commented Nov 13, 2014

Uh oh!

bgrant0607 commented Nov 13, 2014

Uh oh!

smarterclayton commented Nov 13, 2014

Uh oh!

bgrant0607 commented Nov 13, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erictune commented Nov 13, 2014

Uh oh!

bgrant0607 commented Nov 13, 2014

Uh oh!

bgrant0607 commented Nov 13, 2014

Uh oh!

erictune commented Nov 13, 2014

Uh oh!

bgrant0607 commented Nov 13, 2014

Uh oh!

erictune commented Nov 13, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgrant0607 commented Nov 13, 2014

Uh oh!

bgrant0607 commented Nov 13, 2014

Uh oh!

mindscratch commented Nov 13, 2014

Uh oh!

smarterclayton commented Nov 13, 2014

Uh oh!

lavalamp commented Nov 13, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lavalamp commented Nov 13, 2014

Uh oh!

bgrant0607 commented Nov 14, 2014

Uh oh!

bgrant0607 commented Nov 14, 2014

Uh oh!

smarterclayton commented Nov 14, 2014

Uh oh!

bgrant0607 commented Nov 14, 2014