SDI-2573, 2628,2629: Snap API v2 CLIs #2

candysmurf · 2017-03-22T05:06:11Z

Summary of changes

plugin load, unload, list
plugin config get
metric list, get
Added task CLI related implementation using snap-client-go
Refactored task watch locally
Refactored both creating a task from task manifest and workflow to use snap-client-go only.
list running plugins, swap plugins, task export.

Test done

Integration test with snap-client-go and snap

PatrykMatyjasek · 2017-03-22T15:46:45Z

main.go

+http://www.apache.org/licenses/LICENSE-2.0.txt
+
+
+Copyright 2015-2017 Intel Corporation


it should be only 2017

@PatrykMatyjasek, the reason I left 2015-2017 is because that most CLI code base is from Snap repo. What do you think? Keep 2015-2017 or 2017?:-)

@candysmurf It should stay only 2017

@PatrykMatyjasek, changed all to 2017. Please take another look

PatrykMatyjasek · 2017-03-22T15:47:05Z

snaptel/commands.go

+http://www.apache.org/licenses/LICENSE-2.0.txt
+
+
+Copyright 2015-2017 Intel Corporation


it should be only 2017

PatrykMatyjasek · 2017-03-22T15:48:52Z

snaptel/commands.go

+				{
+					Name:        "create",
+					Description: "Creates a new task in the snap scheduler",
+					Usage:       "There are two ways to create a task.\n\t1) Use a task manifest with [--task-manifest]\n\t2) Provide a workflow manifest and schedule details.\n\n\t* Note: Start and stop date/time are optional.\n",


I think there should be all flags described in usage. For example there is specified flag to create task using task manifest but there is lack of options which should be used to run task using workflow and schedule

can you give an example what you want? This is same as what we have in Snap today, Total parity. We can create an issue for your suggestions as future enhancements.

First way of creating task is described with flag which should be used to run. But the other way is not described in such details. I agree about issue :)

PatrykMatyjasek · 2017-03-22T15:49:41Z

snaptel/commands.go

+					Usage:  "list",
+					Action: listTask,
+					Flags: []cli.Flag{
+						flVerbose,


flag is not specified in usage

@PatrykMatyjasek urfave/cli automatically generates help based on provided Flags, output for this command is:

./build/linux/x86_64/snaptel task list --help NAME: snaptel task list - list or list --verbose USAGE: snaptel task list [command options] [arguments...] OPTIONS: --verbose Verbose output

PatrykMatyjasek · 2017-03-22T15:49:57Z

snaptel/commands.go

+					Usage:  "watch <task_id>",
+					Action: watchTask,
+					Flags: []cli.Flag{
+						flVerbose,


missing in usage

PatrykMatyjasek · 2017-03-22T15:56:38Z

snaptel/metric.go

+http://www.apache.org/licenses/LICENSE-2.0.txt
+
+
+Copyright 2015-2017 Intel Corporation


should be only 2017

PatrykMatyjasek · 2017-03-22T16:10:07Z

snaptel/plugin.go

+http://www.apache.org/licenses/LICENSE-2.0.txt
+
+
+Copyright 2015 Intel Corporation


PatrykMatyjasek · 2017-03-22T16:13:00Z

snaptel/plugin.go

+}
+
+func unloadPlugin(ctx *cli.Context) error {
+	pType := ctx.Args().Get(0)


shouldn't we check len of ctx.Args()?

I used existing code that's how I didn't focus on those. I'll add a check.

I was just curious if there could be problems in case of less parameters provided by user

PatrykMatyjasek · 2017-03-22T16:17:35Z

snaptel/task.go

+http://www.apache.org/licenses/LICENSE-2.0.txt
+
+
+Copyright 2015-2017 Intel Corporation


should be 2017

PatrykMatyjasek · 2017-03-22T16:33:13Z

snaptel/watch.go

+http://www.apache.org/licenses/LICENSE-2.0.txt
+
+
+Copyright 2015-2017 Intel Corporation


should be 2017

mkleina · 2017-03-23T14:37:24Z

snaptel/flags.go

+	FlTimeout = cli.DurationFlag{
+		Name:  "timeout, t",
+		Usage: "Timeout to be set on HTTP request to the server",
+		Value: 10 * time.Second,


Is this parameter passed anywhere? Please consider snaptel task watch timeout bug, fixed by this PR:
intelsdi-x/snap#1537

it seems used by this library - github.com/urfave/cli. Not sue how it's used though.

I meant the timeout parameter, I don't see if value of timeout flag is used anywhere, so it is no difference if this tool is used with --timeout=0 or --timeout=9999999?

I see that everywhere in CLI, timeout-less client methods are used, for example:

params := tasks.NewGetTaskParams()

instead of

params := tasks.NewGetTasksParamsWithTimeout(FlTimeout.Value)

so --timeout flag is not used anywhere. I think it should be removed for now if not implemented, or should be implemented in this PR, because it seems to be pretty easy (replace all client methods with <MethodName>WithTimeout(FlTimeout.Value) equivalent)

mkleina · 2017-03-23T14:39:53Z

snaptel/watch.go

+	id := ctx.Args().First()
+	url := fmt.Sprintf("%s/%s/tasks/%s/watch", FlURL.Value, FlAPIVer.Value, id)
+
+	resp, err := http.Get(url)


Shouldn't you set the HTTP timeout here? Please consider snaptel task watch timeout bug, fixed by this PR:
intelsdi-x/snap#1537

@mkleina, a good question. The fix 1537 just set no timeout for watching a task. I'll see what I can do here. Please review my next update.

@mkleina, played with client timeout. Unfortunately, streaming cannot appropriately handle the idle timeout atm. Therefore no timeout can be set.

@candysmurf Original snaptel CLI has --timeout option which sets timeout for all HTTP requests except task watch (which timeout is fixed to 0 after fix 1537), just wanted you to be aware of this timeout setting, because I see http.Get() called here without any timeout configuration.

lmroz · 2017-03-23T17:39:53Z

snaptel/metric.go

+				return err
+			}
+		}
+	} else if len == 1 {


This branch looks unnecessary, preceding condition can be just >0,

@lmroz, cannot believe it passed code review before:-).

lmroz · 2017-03-23T17:51:23Z

snaptel/metric.go

+	}
+	fmt.Printf("\n  Rules for collecting %s:\n\n", namespace)
+	printFields(w, true, 6, "NAME", "TYPE", "DEFAULT", "REQUIRED", "MINIMUM", "MAXIMUM")
+	for _, rule := range metric.Policy {


Is this code printing config policy for each namespace? If so, there will be terrible flood because config policies affect all subnamespaces.

@lmroz, code does what you said which is the current behavior. Feel free to open an issue for future enhancement.

lmroz · 2017-03-23T17:55:49Z

snaptel/metric.go

+
+	if ns != "" {
+		//if the user doesn't provide '/*' we fix it
+		if ns[len(ns)-2:] != "/*" {


What if separator for given namespace is different than /?

@lmroz, let us open an issue for this one as well. It'll break if the separator is not "/".

@lmroz, code seems not used. I removed it.

lmroz · 2017-03-23T18:04:29Z

snaptel/commands.go

+		} else {
+			argArray = append(argArray, "")
+		}
+		if i < (len(fields) - 1) {


This code could be simplified just by moving append to the beginning of for loop and testing i != 0 or just using strings.Join

@lmroz, could a field be nil but not the first one?

I don't see any early return depending on if its nil or not so inserting tab after each element but last one is equivalent to inserting tab before each element but first one.

then it needs to check if it's the first one. No saving :-)

candysmurf · 2017-04-03T02:58:11Z

@PatrykMatyjasek, @lmroz, @intelsdi-x/snap-maintainers, this PR incorporated all your code reviews. Would you please take another look.

candysmurf · 2017-04-03T17:01:48Z

@intelsdi-x/snap-maintainers, PR in snap-client-go is merged. It makes the testing of this PR much easier. Please test this PR against the PR submitted in Snap PR 1535

mkleina · 2017-05-30T13:45:00Z

snaptel/common.go

+func getErrorDetail(err error, ctx *cli.Context) error {
+	switch err.(type) {
+	case *plugins.GetMetricsNotFound:
+		return newUsageError(fmt.Sprintf("%v", err.(*plugins.GetMetricsNotFound).Payload.Message), ctx)


Maybe my fault but I am getting error messages here:

snaptel/common.go:71: err.(*plugins.GetMetricsNotFound).Payload.Message undefined (type *models.Error has no field or method Message) snaptel/common.go:73: err.(*plugins.GetMetricsInternalServerError).Payload.Message undefined (type *models.Error has no field or method Message) snaptel/common.go:75: err.(*plugins.LoadPluginBadRequest).Payload.Message undefined (type *models.Error has no field or method Message) snaptel/common.go:77: err.(*plugins.LoadPluginConflict).Payload.Message undefined (type *models.Error has no field or method Message) snaptel/common.go:79: err.(*plugins.LoadPluginInternalServerError).Payload.Message undefined (type *models.Error has no field or method Message) snaptel/common.go:81: err.(*plugins.LoadPluginUnsupportedMediaType).Payload.Message undefined (type *models.Error has no field or method Message) snaptel/common.go:83: err.(*plugins.UnloadPluginBadRequest).Payload.Message undefined (type *models.Error has no field or method Message) snaptel/common.go:85: err.(*plugins.UnloadPluginConflict).Payload.Message undefined (type *models.Error has no field or method Message) snaptel/common.go:87: err.(*plugins.UnloadPluginInternalServerError).Payload.Message undefined (type *models.Error has no field or method Message) snaptel/common.go:89: err.(*plugins.UnloadPluginNotFound).Payload.Message undefined (type *models.Error has no field or method Message)

Payload is models.Error in all cases, for example:

type GetMetricsNotFound struct { Payload *models.Error }

But there are no Message field for models.Error:
https://github.com/candysmurf/snap-client-go/blob/2017-05-16/models/error.go

There are only ErrorMessage field.

@mkleina, fixed it.

mkleina · 2017-05-30T13:49:53Z

snaptel/plugin.go

+
+		printFields(w, false, 0, "NAME", "HIT COUNT", "LAST HIT", "TYPE", "PPROF PORT")
+		for _, rp := range resp.Payload.Plugins {
+			printFields(w, false, 0, rp.Name, rp.Hitcount, time.Unix(rp.LastHitTimestamp, 0).Format(time.RFC1123), rp.Type, rp.PprofPort)


Should be rp.HitCount:

snaptel/plugin.go:141: rp.Hitcount undefined (type *models.Plugin has no field or method Hitcount, but does have HitCount)

mkleina · 2017-05-31T08:37:20Z

snaptel/watch.go

+			break
+		}
+	}
+	return nil


Please remove all w.Flush() statements and insert final w.Flush() just before return nil. Flush() should be called once after everything was printed to buffer, otherwise column formatting won't be correct.

mkleina · 2017-05-31T13:24:29Z

main.go

+	app.Version = gitversion
+	app.Usage = "The open telemetry framework"
+	app.Flags = []cli.Flag{snaptel.FlURL, snaptel.FlSecure, snaptel.FlAPIVer, snaptel.FlPassword, snaptel.FlConfig, snaptel.FlTimeout}
+	app.Setup()


Please move app.Setup() just before app.Run() after setting all app fields. Otherwise, snaptel --help does not print out available commands.

jcooklin · 2017-06-02T20:46:01Z

glide.yaml

@@ -0,0 +1,55 @@
+package: github.com/intelsdi-x/snap


This should be github.com/intelsdi-x/snap-cli

jcooklin · 2017-06-02T20:48:43Z

LGTM - after updating the package name at the top of the glide.yaml.

Changed.

PatrykMatyjasek · 2017-06-05T06:39:32Z

LGTM

PatrykMatyjasek previously requested changes Mar 22, 2017

View reviewed changes

candysmurf changed the title ~~Task watch~~ SDI-2629: Task CLIs Mar 23, 2017

mkleina suggested changes Mar 23, 2017

View reviewed changes

lmroz reviewed Mar 23, 2017

View reviewed changes

candysmurf force-pushed the task-watch branch 3 times, most recently from f9fa285 to 0355221 Compare April 3, 2017 02:43

candysmurf changed the title ~~SDI-2629: Task CLIs~~ SDI-2628,2629: Snap API v2 CLIs Apr 3, 2017

candysmurf changed the title ~~SDI-2628,2629: Snap API v2 CLIs~~ SDI-2573, 2628,2629: Snap API v2 CLIs Apr 3, 2017

candysmurf mentioned this pull request Apr 3, 2017

SDI-2573:Create initial implementation of snap-cli #1

Closed

candysmurf force-pushed the task-watch branch 3 times, most recently from 549a7ac to ad4f82d Compare April 6, 2017 16:14

candysmurf force-pushed the task-watch branch from ad4f82d to 630463c Compare May 17, 2017 18:03

mkleina reviewed May 30, 2017

View reviewed changes

mkleina suggested changes May 30, 2017

View reviewed changes

mkleina suggested changes May 31, 2017

View reviewed changes

candysmurf force-pushed the task-watch branch 2 times, most recently from 635ac92 to 582f6ab Compare June 2, 2017 16:06

mkleina approved these changes Jun 2, 2017

View reviewed changes

jcooklin reviewed Jun 2, 2017

View reviewed changes

glide.yaml Outdated

@@ -0,0 +1,55 @@

package: github.com/intelsdi-x/snap

Copy link

Collaborator

jcooklin Jun 2, 2017

Choose a reason for hiding this comment

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

This should be github.com/intelsdi-x/snap-cli

Snap CLI for go-swagger generated RestAPI client

c83b7c0

candysmurf force-pushed the task-watch branch from 3f6d883 to c83b7c0 Compare June 3, 2017 01:01

candysmurf merged commit 3dc8098 into intelsdi-x:master Jun 5, 2017

candysmurf deleted the task-watch branch June 5, 2017 16:11

		http://www.apache.org/licenses/LICENSE-2.0.txt


		Copyright 2015-2017 Intel Corporation

		http://www.apache.org/licenses/LICENSE-2.0.txt


		Copyright 2015 Intel Corporation

SDI-2573, 2628,2629: Snap API v2 CLIs #2

SDI-2573, 2628,2629: Snap API v2 CLIs #2

Conversation

candysmurf commented Mar 22, 2017 • edited Loading

Summary of changes

Test done

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

mkleina Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkleina May 31, 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

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 Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candysmurf commented Apr 3, 2017

candysmurf commented Apr 3, 2017

mkleina May 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkleina May 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcooklin commented Jun 2, 2017

PatrykMatyjasek commented Jun 5, 2017

candysmurf commented Mar 22, 2017 •

edited

Loading

mkleina Mar 23, 2017 •

edited

Loading

mkleina May 31, 2017 •

edited

Loading

candysmurf Mar 23, 2017 •

edited

Loading

mkleina May 30, 2017 •

edited

Loading

mkleina May 31, 2017 •

edited

Loading