Document Snap with valid swagger spec #1599

candysmurf · 2017-04-11T21:18:00Z

Fixes #1573

Summary of changes:

Added swagger-go annotations to Snap RestAPI v2
Consolidate data models for plugins and tasks
Renamed a few variables to make go-swagger compliance

Testing done:

make test-small
make test-medium
make test-legacy

Note:
go-swagger generated client does not work with snap perfectly. The next PR will address the issue #1600

@intelsdi-x/snap-maintainers

candysmurf · 2017-04-11T21:23:10Z

mgmt/rest/v2/config.go

 type PluginConfigItem struct {
-	cdata.ConfigDataNode
+	// in: body
+	Config cdata.ConfigDataNode `json:"config"`


Exposed the config so that clients in different repositories/anywhere can reference it.

candysmurf · 2017-04-11T21:28:17Z

mgmt/rest/v2/mock/mock_task_manager.go

@@ -179,7 +179,7 @@ const (
      "deadline": "4ns",
      "creation_timestamp": -62135596800,
      "last_run_timestamp": -1,
-      "task_state": "Running",
+      "state": "Running",


changed task_state to state so that the JSON representation(currently it's task_state) is same as the variable name (state). The reason is:

go-swagger prefers the variable name to be same as its JSON representation.

task_state only appears in the Snap response but not as any input. It's not a breaking change.

state is inside task struct. The name task_state is redundant.

We cannot change the response of the API after it's landed (V1 or V2). Please revert this back to task_state.

candysmurf · 2017-04-11T21:32:19Z

mgmt/rest/v2/plugin.go

+	LastHitTimestamp int64         `json:"last_hit_timestamp,omitempty"`
+	ID               uint32        `json:"id,omitempty"`
+	PprofPort        string        `json:"pprof_port,omitempty"`
+}


Merged RunningPlugin with 'Plugin` data type. The difference can be omitted in the JSON representation in terms of different plugin response scenarios. The reason is to simplify the plugin data model in swagger spec.

Will this change the response, causing a breaking change?

No, it'll not change responses. Please test yourself and let me know if you find anything different.

candysmurf · 2017-04-11T21:36:10Z

mgmt/rest/v2/task.go

+	State              string            `json:"state,omitempty"`
+	Href               string            `json:"href,omitempty"`
+	Start              bool              `json:"start,omitempty"`
+	MaxFailures        int               `json:"max-failures,omitempty"`


Consolidated Task data model from Snap CLI and Rest API so that they reference the same Task data model in Swagger spec. The benefit is that no business logic in CLIs/clients and Snap server defines the client behaviors universally.

candysmurf · 2017-04-11T21:40:25Z

scheduler/wmap/wmap.go

-	PublishNodes []PublishWorkflowMapNode `json:"publish,omitempty"yaml:"publish"`
+	// required: true
+	PluginName    string                   `json:"plugin_name"yaml:"plugin_name"`
+	PluginVersion int                      `json:"plugin_version"yaml:"plugin_version"`


Changed Name to PluginName and Version to PluginVersion for the consistency between variable names and JSON representations. The consequence is:

go-swagger generated client can reference back easily and no patching shell scripts are necessary.

variable name changes are invisible to customers. Changes only affect tests inside code base.

candysmurf · 2017-04-11T22:01:32Z

Issue #1600 will be addressed in the subsequence PRs.

candysmurf · 2017-04-12T18:28:23Z

@jcooklin, thanks for reviewing it. task_state is reverted back. If you like to take another look.

candysmurf · 2017-04-12T18:30:25Z

@intelsdi-x/snap-maintainers, this PR has been there for awhile. It affects subsequence work related to this initiative. Your timely code review is greatly appreciated.

kjlyon

I left a few questions in the comments. I am still running tests to ensure v1 was not affected by your changes.

kjlyon · 2017-04-13T18:20:42Z

.travis.yml

 language: go
 go:
 - 1.7.3
 before_install:
 - bash scripts/gitcookie.sh
 - go get github.com/smartystreets/goconvey/convey
+- echo "deb https://dl.bintray.com/go-swagger/goswagger-debian ubuntu main" | sudo tee -a /etc/apt/sources.list
+- sudo apt-get update && sudo apt-get install -y swagger --force-yes


I had to manually install go-swagger. What does this section do? sudo apt-get install -y swagger --force-yes

this script installs swagger to travis env.

kjlyon · 2017-04-13T18:29:25Z

core/schedule.go

-	Interval       string     `json:"interval,omitempty"`
+	// required: true
+	// enum: simple, windowed, streaming, cron
+	Type string `json:"type"`


Why not Type string json:"type,omitempty"

it's beneficial that users can select a type from an available GUI list instead of by leaving it empty although we could default it to simple. It's not clear when we have public exposing APIs.

of course, for the consistent reason or whatever, if team insists to leave it as not required. I will do that but adding a default value.

kjlyon · 2017-04-13T18:34:43Z

mgmt/rest/v2/api.go

@@ -56,25 +56,302 @@ func New(wg *sync.WaitGroup, killChan chan struct{}, protocol string) *apiV2 {

 func (s *apiV2) GetRoutes() []api.Route {
 	routes := []api.Route{
-		// plugin routes
+		// swagger:route GET /plugins plugins getPlugins
+		//


Is there any way these comments can be compressed? I find them distracting from the actual code.

No, comments have to stay like this format in order for them to show up in UI pretty. Otherwise, it shows up but not as pretty as it's.

kjlyon · 2017-04-13T19:09:51Z

mgmt/rest/v2/metric.go

+// MetricsResp is the representation of metric operation response.
+//
+// swagger:response MetricsResponse
+type MetricsResp struct {


What is the purpose of adding these sections?

The purpose for this is that APIs have appropriate response types.

kjlyon · 2017-04-13T19:14:29Z

mgmt/rest/v2/metric.go

-	Href                    string           `json:"href"`
+	LastAdvertisedTimestamp int64 `json:"last_advertised_timestamp,omitempty"`
+	//required: true
+	Namespace string `json:"namespace"`


Why not omitempty?

Do you agree the namespace of a metric should be required? Would you please let me know in what use case where a JSON/YAML representation won't specify the namespace for a metric?

kjlyon · 2017-04-13T21:35:04Z

mgmt/rest/v2/plugin.go

+	LastHitTimestamp int64         `json:"last_hit_timestamp,omitempty"`
+	ID               uint32        `json:"id,omitempty"`
+	PprofPort        string        `json:"pprof_port,omitempty"`
+}


Will this change the response, causing a breaking change?

kjlyon · 2017-04-13T21:47:05Z

scheduler/wmap/wmap.go

@@ -365,9 +369,10 @@ func (p *ProcessWorkflowMapNode) GetConfigNode() (*cdata.ConfigDataNode, error)
 }

 type PublishWorkflowMapNode struct {
-	Name    string `json:"plugin_name"yaml:"plugin_name"`
-	Version int    `json:"plugin_version"yaml:"plugin_version"`
-	// TODO publisher config


This TODO was taken out. Unless you did this work, it should be added back in.

I can add it back but I really don't think it's still relevant. We can specify a config within a publisher.

IzabellaRaulin · 2017-04-13T21:58:29Z

swagger.json

+      "name": "Apache 2.0",
+      "url": "http://www.apache.org/licenses/"
+    },
+    "version": "1.1.0"


I have a question about the version - it looks like the version of Snap (however the latest one is 1.2.0, not 1.1.0). How should it be managed when a new release is coming? Or maybe it's rather about version of Snap Restful API? Could you explain me that? Thank You

good catch, It should be the version of our API.

IzabellaRaulin · 2017-04-13T22:00:53Z

swagger.json

+      }
+    },
+    "TasksResponse": {
+      "description": "TasksResp returns a list of created tasks.",


In description - TasksResp or TasksResponse? A similar thing in line 1316 and others places across this file.

IzabellaRaulin · 2017-04-13T22:09:22Z

swagger.json

+      ],
+      "properties": {
+        "config": {
+          "description": "TODO processor config",


This one should not have such description. Of course, this points that this line should be changed.

IzabellaRaulin · 2017-04-13T22:17:25Z

mgmt/rest/v2/config.go

 }

 func (s *apiV2) getPluginConfigItem(w http.ResponseWriter, r *http.Request, p httprouter.Params) {
 	var err error
 	styp := p.ByName("type")
 	if styp == "" {
 		cdn := s.configManager.GetPluginConfigDataNodeAll()
-		item := &PluginConfigItem{ConfigDataNode: cdn}
+		item := &PluginConfigItem{Config: cdn}


Small suggestion: changing cdn to for example cfg
Why: after changing the name ConfigDataNode to Config, cdn does not make sense as it had before.

IzabellaRaulin · 2017-04-13T22:17:48Z

mgmt/rest/v2/config.go

@@ -65,7 +95,7 @@ func (s *apiV2) getPluginConfigItem(w http.ResponseWriter, r *http.Request, p ht
 	}

 	cdn := s.configManager.GetPluginConfigDataNode(typ, name, iver)
-	item := &PluginConfigItem{ConfigDataNode: cdn}
+	item := &PluginConfigItem{Config: cdn}


As above, small suggestion: changing cdn to for example cfg
Why: after changing the name ConfigDataNode to Config, cdn does not make sense as it had before.

IzabellaRaulin · 2017-04-13T22:22:47Z

mgmt/rest/v2/task.go

-	ID                 string            `json:"id"`
-	Name               string            `json:"name"`
-	Deadline           string            `json:"deadline"`
+	Version            int               `json:"version,omitempty"`


Does Task has own version? Is it not a mistake?

yes, currently task has its own version if you look the task manifest. Does the task version make sense? idk.

You are right - task has a version used to differentiate between versions of the task manifest parser (right now, there is only one version: 1). For others reviewers - you can find more about it here

I have just a concern why Task struct did not have Version field before, but I need to be more acquainted with the flow of task version to say sth more.

@candysmurf - what do you think about moving Version after ID and Name? Why: task ID and its name identified the task, so a good pattern keeps those two in the beginning. This is only a small suggestion, so up to you.

@IzabellaRaulin, changed the version as suggested, please let me know if you have other suggestions.

IzabellaRaulin · 2017-04-13T22:32:38Z

swagger.json

+      }
+    },
+    "PluginConfigResponse": {
+      "description": "PluginConfigItem represents the response of a plugin config items.",


@candysmurf, please verify if the description is as you expected. I have a doubt about PluginConfigItem - maybe it should be PluginConfigResponse.

jcooklin · 2017-04-25T22:52:53Z

mgmt/rest/v2/mock/mock_config_manager.go

@@ -70,19 +70,25 @@ func (MockConfigManager) DeletePluginConfigDataNodeFieldAll(fields ...string) cd
 // rest_v2_test.go on the plugin config routes found in mgmt/rest/server.go
 const (
 	SET_PLUGIN_CONFIG_ITEM = `{
-  "user": "Jane"
+  "config": {


Can we avoid this breaking API change?

@jcooklin, the response change is from word body to config. if that's your concern, I can change it to body. In this context, config makes more sense. Please try it. With API V1, after set config, the response is:

V1 API response after updating a config item:

{ "meta": { "code": 200, "message": "Plugin config item(s) set", "type": "config_plugin_item_created", "version": 1 }, "body": { "leader": "thunder", "password": "p@ssw0rd", "path": "whatever", "user": "whoever" } }

V2 API response after updating a config item:

{ "config": { "leader": "thunder", "password": "p@ssw0rd", "path": "whatever", "user": "whoever" } }

@jcooklin, I tried to remove "config" totally. Currently, there is no way to do that in go-swagger. The only way is to add a wrap in snap-client-go to return the response we wanted. Please let me know if you like to do that?

mkleina · 2017-04-27T13:49:48Z

scripts/swagger.sh

+
+swagger generate spec -o ${__dir}/../swagger.json
+
+perl -i -p -e 's/ProcessNodes/Process/g' ${__dir}/../swagger.json


Is it really needed to rename ProcessNodes to Process? I think that we shouldn't modify auto-generated files.

@mkleina, otherwise generated client does not work with Snap server as the data model is different. Unless we rename our variables inside Snap code base. I don't think @jcooklin let me do that.

mkleina · 2017-04-27T13:53:57Z

swagger.json

+        "process": {
+          "type": "array",
+          "items": {
+            "$ref": "#/definitions/ProcessWorkflowMapNode"


There is circular reference, which causes swagger-ui to crash (RangeError: Maximum call stack size exceeded). Here is the definition of ProcessWorkflowMapNode:

type ProcessWorkflowMapNode struct { // required: true PluginName string `json:"plugin_name"yaml:"plugin_name"` PluginVersion int `json:"plugin_version"yaml:"plugin_version"` ProcessNodes []ProcessWorkflowMapNode `json:"process,omitempty"yaml:"process"` PublishNodes []PublishWorkflowMapNode `json:"publish,omitempty"yaml:"publish"` // Config the configuration of a processor. Config map[string]interface{} `json:"config,omitempty"yaml:"config"` Target string `json:"target"yaml:"target"` }

NOTE: Works OK with older swagger-ui 2.2.9, error on swagger-ui 3.0.5

@jcooklin Should we fix that somehow or wait for next swagger-ui revision (probably bug in v3.0.5 -
swagger-api/swagger-ui#2889)?

I think we should wait for the next release. The issue you referenced looks promising in that they appear to be addressing it.

mkleina · 2017-05-08T12:10:08Z

mgmt/rest/v2/config.go

+// PluginConfigResponse represents the response of a plugin config items.
+//
+// swagger:response PluginConfigResponse
+type PluginConfigResponse struct {


@candysmurf PluginConfigResponse is not used anywhere, but it will change in future PR, do I understand correctly?

This provides the (generated) client with a response object for plugin config calls. You will notice that this struct is referenced in api.go (e.g. here).

Ok, so all Response structs are just for declaring swagger specification and generating client, now I understand. I was confused cause I was looking on /plugins call swagger tags:

// Responses: // 200: PluginsResponse

and getPlugins() handler method:

Write(200, PluginsResponse{Plugins: filteredPlugins}, w)

and in this case the response struct passed to Write() is the same as in swagger tags, but now I see that even in this case, the "real" PluginsResponse for generating client is actually PluginsResp:

// PluginsResp represents the response from plugins operations. // // swagger:response PluginsResponse type PluginsResp struct { // List of plugins // // in: body Body struct { Plugins []Plugin `json:"plugins,omitempty"` } }

@mkleina, glad that you got it!

mkleina · 2017-05-08T12:43:20Z

mgmt/rest/v2/error.go

+// swagger:response ErrorResponse
+type ErrorResponse struct {
+	// in:body
+	SnapError Error `json: "snap_error""`


@candysmurf Mistype here probably, double "

mkleina · 2017-05-08T15:26:20Z

mgmt/rest/v2/config.go

@@ -105,7 +139,7 @@ func (s *apiV2) deletePluginConfigItem(w http.ResponseWriter, r *http.Request, p
 		res = s.configManager.DeletePluginConfigDataNodeField(typ, name, iver, src...)
 	}

-	item := &PluginConfigItem{ConfigDataNode: res}
+	item := &PluginConfigItem{res}


@candysmurf Will this (and all other responses) be changed to PluginConfigResponse in the future PR?

It's unlikely IMO. Like mentioned above the response struct is to effectively support generating the client.

jcooklin · 2017-05-08T15:54:05Z

@candysmurf once @mkleina finishes reviewing please squash the commits.

changed JSON task state back to task_state added the script to remove the sed command generated swagger.jsone file addressed code review feedback 1 removed the perl replacement in swagger.sh and related variable renames added multiform-data for post & put Use Body to hide config made the annotation accurate for consumers and producers plus changed formData to body annotation remove typo

candysmurf commented Apr 11, 2017

View reviewed changes

candysmurf force-pushed the swagger-1 branch from de3daa4 to 5bc5fe7 Compare April 11, 2017 21:41

candysmurf force-pushed the swagger-1 branch from 6947646 to a8510d0 Compare April 13, 2017 21:36

kjlyon reviewed Apr 13, 2017

View reviewed changes

IzabellaRaulin reviewed Apr 13, 2017

View reviewed changes

candysmurf force-pushed the swagger-1 branch 2 times, most recently from f5587f0 to 917730a Compare April 17, 2017 19:20

jcooklin suggested changes Apr 25, 2017

View reviewed changes

mkleina reviewed Apr 27, 2017

View reviewed changes

candysmurf force-pushed the swagger-1 branch 2 times, most recently from 1d4eec2 to c12e4c9 Compare May 3, 2017 21:11

mkleina reviewed May 8, 2017

View reviewed changes

mkleina suggested changes May 8, 2017

View reviewed changes

jcooklin approved these changes May 8, 2017

View reviewed changes

candysmurf force-pushed the swagger-1 branch from c12e4c9 to 5fc1b1f Compare May 10, 2017 03:17

mkleina approved these changes May 10, 2017

View reviewed changes

candysmurf force-pushed the swagger-1 branch from 5fc1b1f to 7aff489 Compare May 12, 2017 17:53

candysmurf merged commit 3addb66 into intelsdi-x:master May 12, 2017


		swagger generate spec -o ${__dir}/../swagger.json

		perl -i -p -e 's/ProcessNodes/Process/g' ${__dir}/../swagger.json

Document Snap with valid swagger spec #1599

Document Snap with valid swagger spec #1599

Conversation

candysmurf commented Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

candysmurf Apr 11, 2017 • edited Loading

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

candysmurf Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

candysmurf commented Apr 11, 2017

candysmurf commented Apr 12, 2017

candysmurf commented Apr 12, 2017

kjlyon left a 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

IzabellaRaulin Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

IzabellaRaulin Apr 13, 2017 • edited Loading

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

jcooklin Apr 25, 2017 • edited Loading

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

mkleina Apr 27, 2017 • edited Loading

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

jcooklin commented May 8, 2017

candysmurf commented Apr 11, 2017 •

edited

Loading

candysmurf Apr 11, 2017 •

edited

Loading

candysmurf Apr 11, 2017 •

edited

Loading

IzabellaRaulin Apr 13, 2017 •

edited

Loading

IzabellaRaulin Apr 13, 2017 •

edited

Loading

jcooklin Apr 25, 2017 •

edited

Loading

mkleina Apr 27, 2017 •

edited

Loading