A huge migration to a newer swagger. #3481

vburenin · 2016-12-09T22:26:51Z

What can I say? It is a HUGE change. Full CI passed, which is exciting and scary at the same time. Not sure there is a good way to review it all carefully, pick a part you are familiar with and see what happens.

Fixes: #929

mdubya66 · 2016-12-11T19:17:38Z

fixes #929 ?

hmahmood · 2016-12-12T16:40:31Z

lib/apiservers/engine/backends/container_test.go

@@ -615,7 +616,7 @@ func TestPortInformation(t *testing.T) {
 	mockContainerInfo := &plmodels.ContainerInfo{}
 	mockContainerConfig := &plmodels.ContainerConfig{}
 	containerID := "foo"
-	mockContainerConfig.ContainerID = &containerID
+	mockContainerConfig.ContainerID = containerID


There are a lot of these changes where we are taking away the pointer. Can we not do these in a separate PR; preferably in an iterative manner? These kinds of changes always have the danger of introducing subtle bugs.

This is the least problematic change you can think of - at least for these values I was able to check the code surrounding it. The most scariest part is when I set "x-nullable":false - this is where zero values are no longer accepted. I tried to be careful with them, but things that are changed are not as scary as those that are unchanged.

Well, yes, this one in particular is not problematic. But that doesn't mean the multitude of others is not; its just not easy to go through everyone and understand the ramifications of a particular change. Is the "x-nullable" really necessary right now?

@hmahmood without it the change would be an order of a magnitude larger.

@hmahmood the good part of this change is that it will expose problematic parts of our API and data checks earlier. What really worries me is that we do not really care about data validation relaying purely on a sane usage. At the moment it is fine, but AFAIK PortLayer API will be exposed in the future, so absence of data validation will bite us.

mdubya66 · 2016-12-12T17:54:26Z

My take on this is let's get it tested as much as possible. Run against the CI, the Nightlies, Stress and Longevity. We should aim to get it in sooner so that we can continue to iterate on it.

sflxn · 2016-12-12T19:11:48Z

lib/apiservers/engine/backends/container.go

-	if container == nil {
-		log.Errorf("Could not find container with ID %s", *t.ContainerConfig.ContainerID)
+	cid := t.ContainerConfig.ContainerID
+	c := cache.ContainerCache().GetContainer(cid)


+1. I really hate go programmer's habit of overloading one line with a bazillion operations. They need to remember the compiler doesn't care, and readability is more important.

fdawg4l · 2016-12-12T19:20:17Z

lib/apiservers/engine/backends/backends.go


 	"github.com/vmware/vic/lib/apiservers/engine/backends/cache"
 	"github.com/vmware/vic/lib/apiservers/engine/backends/container"
-	"github.com/vmware/vic/lib/apiservers/portlayer/client"
+	apiclient "github.com/vmware/vic/lib/apiservers/portlayer/client"


apiclient isn't really necessary to migrate from 1 swagger version to another. We can debate the naming of this later. Lets keep this change restricted only only the required changes to include the new swagger version. The unnecessary churn makes reviewing harder.

I used tools to deal with imports, all automatic changes are not easy to manage manually.

fdawg4l · 2016-12-12T19:21:00Z

lib/apiservers/engine/backends/backends.go

@@ -123,52 +124,52 @@ func hydrateCaches() error {

 	wg := sync.WaitGroup{}
 	wg.Add(waiters)
-	errors := make(chan error, waiters)
+	errChan := make(chan error, waiters)


Same comment as above. This isn't strictly necessary to migrate to the new swagger version. Please remove it from your change and we can review these cosmetic changes in a different PR.

This is the cosmetic change and the only one that you can say "big".

fdawg4l · 2016-12-12T19:21:51Z

lib/apiservers/engine/backends/container.go

@@ -985,22 +984,25 @@ func (c *Container) ContainerInspect(name string, size bool, version version.Ver
 	}
 	var started time.Time
 	var stopped time.Time
-	if results.Payload.ProcessConfig.StartTime != nil && *results.Payload.ProcessConfig.StartTime > 0 {


Is this purely formatting? Unnecessary churn to migrate to the new swagger. Encapsulate these cosmetic changes in their own PR and we can review them later.

This is not a cosmetic change. it a result of a variable becoming a value not a pointer during migration.

@fdawg4l There are a few cosmetic changes, but I would gloss over them and focus on the pointer issue. Vast majority of the changes I've seen so far are due to Swagger and not cosmetic.

…-update

This reverts commit 7ede85f.

This reverts commit 7b49636.

caglar10ur · 2016-12-12T21:14:06Z

lib/apiservers/engine/backends/system.go

-		if vchInfo.Memory != nil {
-			info.MemTotal = *vchInfo.Memory << 20 //Multiply by 1024*1024 to get Mebibytes
+		if vchInfo.Memory > 0 {
+			info.MemTotal = vchInfo.Memory * 1024 * 1024 // Get Mebibytes


come one - 2^20 FTW :)

I'd prefer to keep it as is also.

come one - compiler will do the same thing.

ok, if go compiler is smart to do this. Still hurts the eyes of former C/C++ programmers. :)

caglar10ur · 2016-12-12T21:16:51Z

lib/apiservers/templates/server/server.gotmpl

@@ -1,225 +0,0 @@
-package {{ .APIPackage }}


we needed this for 030764a . How do we handle it with new swagger?

I need to verify it is still the case.

#1244 is the original issue for your reference

@caglar10ur Thanks for a good catch. I am going to backport this change directly into swagger, since it is actually makes sense. I was about to implement same thing on my previous projects.

@caglar10ur I integrated needed changes into swagger as well as updated portlayer accordingly.
go-swagger/go-swagger#784

sflxn · 2016-12-12T21:58:22Z

lib/apiservers/portlayer/restapi/handlers/misc_handlers.go

 }

 // Ping sends an OK response to let the client know the server is up
-func (handler *MiscHandlersImpl) Ping() middleware.Responder {
+func (h *MiscHandlersImpl) Ping(param misc.PingParams) middleware.Responder {


Parameter-less operations in the swagger spec now causes parameters to be generated?

yes, they contain some useful objects like context, request, etc.

sflxn · 2016-12-12T22:01:37Z

lib/apiservers/portlayer/restapi/handlers/scopes_handlers.go

 	sc := &models.ScopeConfig{
-		ID:        &id,
+		ID:        scope.ID().String(),


Seems a little inconsistent. Some places, you're introducing new vars for readability, but then you remove them in other places.

In this case it makes sense, since it is a short string, I can see all at once.

sflxn · 2016-12-12T22:12:14Z

lib/apiservers/portlayer/swagger.json

-					"type": "integer",
-					"format": "int64"
+					"x-nullable": false,
+					"type": "integer"


Why is this a type integer instead of int64 now?

format != type.

The version of Swagger we used caused an int64 to get generated if we tag the parameter with format. Latest version of Swagger no longer does this?

sflxn · 2016-12-12T22:14:30Z

lib/apiservers/portlayer/swagger.json

 					"type": "string"
 				},
 				"createTime": {
-					"type": "integer",
-					"format": "int64"
+					"type": "integer"


Is Swagger defaulting to in64 now? I believe unix time is int64.

it always was :-|

if it wasn't it would not compile.

sflxn · 2016-12-12T22:15:18Z

lib/apiservers/portlayer/swagger.json

@@ -2721,8 +2763,7 @@
 					"$ref": "#/definitions/ReservationConfig"
 				},
 				"storageSize": {
-					"type": "integer",
-					"format": "int64"
+					"type": "integer"


Same as above. Why integer instead of int64?

After our slack conversation, I see that integer now defaults to int64 in the latest Swagger generator.

sflxn · 2016-12-12T22:29:40Z

Nothing seemed out of the ordinary. We'll just have to be aware of x-nullable and integer generation changes. Is this the extent of all the changes necessary?

lgtm

caglar10ur · 2016-12-13T17:59:48Z

LGTM

mdubya66 · 2016-12-13T19:29:56Z

LGTM

Huge migration to a newer swagger.

922b081

vmwclabot added the cla-not-required label Dec 9, 2016

Update Makefile according to swagger changes.

a233d31

vburenin changed the title ~~[full ci] Huge migration to a newer swagger. DO NOT REVIEW YET.~~ [full ci] Huge migration to a newer swagger. Dec 10, 2016

vburenin changed the title ~~[full ci] Huge migration to a newer swagger.~~ [full ci] A huge migration to a newer swagger. Dec 12, 2016

hmahmood reviewed Dec 12, 2016

View reviewed changes

sflxn reviewed Dec 12, 2016

View reviewed changes

fdawg4l reviewed Dec 12, 2016

View reviewed changes

vburenin added 5 commits December 12, 2016 14:13

Swagger migration pure code related changes.

7ede85f

All new swagger vendor code.

7b49636

Merge branch 'swagger-update' of github.com:vburenin/vic into swagger…

d360ed4

…-update

Revert "Swagger migration pure code related changes."

7988621

This reverts commit 7ede85f.

Revert "All new swagger vendor code."

737c885

This reverts commit 7b49636.

caglar10ur reviewed Dec 12, 2016

View reviewed changes

sflxn reviewed Dec 12, 2016

View reviewed changes

Updated Makefile.

3445742

sflxn reviewed Dec 12, 2016

View reviewed changes

vburenin added 3 commits December 13, 2016 01:44

Provide a user hook for api servers configuration in swagger.

402830e

Use api server config hooks to set custom shutfown workflow,.

8110091

Merge branch 'master' into swagger-update

b352166

vburenin changed the title ~~[full ci] A huge migration to a newer swagger.~~ A huge migration to a newer swagger. Dec 13, 2016

Merge branch 'master' into swagger-update

142f93b

vburenin merged commit f57cd75 into vmware:master Dec 13, 2016

vburenin deleted the swagger-update branch December 14, 2016 06:07

A huge migration to a newer swagger. #3481

A huge migration to a newer swagger. #3481

Conversation

vburenin commented Dec 9, 2016 • edited Loading

mdubya66 commented Dec 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vburenin Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdubya66 commented Dec 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sflxn commented Dec 12, 2016

caglar10ur commented Dec 13, 2016

mdubya66 commented Dec 13, 2016

vburenin commented Dec 9, 2016 •

edited

Loading

mdubya66 commented Dec 11, 2016 •

edited

Loading

vburenin Dec 12, 2016 •

edited

Loading