Use go-cli lib #378

carlosms · 2018-12-03T17:01:35Z

Migrate the CLI to use https://github.com/src-d/go-cli.

This PR does not solve any issue, but I think in the long term it's good to have all our projects use the same libraries to be consistent.

cmd/lookout-sdk/event.go

cmd/lookout-sdk/main.go

cmd/lookout-sdk/push.go

cmd/lookout-sdk/review.go

cmd/lookoutd/common.go

cmd/lookoutd/migrate.go

cmd/lookoutd/serve.go

cmd/lookoutd/watch.go

cmd/lookoutd/work.go

smacker · 2018-12-04T10:55:16Z

cmd/dummy/main.go

 )

+var app = gocli.New("dummy", version, build, "Dummy analyzer for testing")


gocli.New(name, version, build, "Dummy analyzer for testing")

Done in 0c2b3ea

smacker · 2018-12-04T10:58:36Z

util/cli/log.go

-
-// Init initializes the default logger factory.
-func (c *LogOptions) init(app *App) {
+// InitLog is similar to go-cli LogOptions.Init, but adds the application name


it also applies the configuration to logrus.

Done in f800074

smacker · 2018-12-04T11:13:25Z

cmd/lookoutd/serve.go

 	queueConsumerCommand
 }

 func (c *ServeCommand) Execute(args []string) error {
 	ctx, stopCtx := context.WithCancel(context.Background())
 	stopCh := make(chan error, 1)

+	cli.InitLog(&c.LogOptions, name)


What do you think if we use cli.Command instead of cli.PlainCommand? It would bring profiling by default now and any other perks in the future. Also I really like that go-cli calls init by default. Right now we don't use it.

My suggestion:

type LookoutCommand struct { cli.Command } func (c LookoutCommand) Init(a *App) error { c.Command.Init() // add additional logic for our logger }

I'd prefer to leave the full cli.Command out of this PR to limit it to migrate what we have to go-cli, as is. We can add profiling later easily if we think we need it.

For the initializer interface, I added it in 2812a43

makes sense 👍

smacker · 2018-12-04T11:14:52Z

cmd/lookoutd/serve.go

 }

 type ServeCommand struct {
+	gocli.PlainCommand `name:"serve" short-description:"run a standalone server" long-description:"Run a standalone server"`
 	queueConsumerCommand
 }

 func (c *ServeCommand) Execute(args []string) error {


Let's use ExecuteContext and remove our custom signal handling.

Done in 5efef51

smacker

There are few inline comments. But also maybe we should change our private init functions to use Initializer where it's appropriate.

carlosms · 2018-12-05T16:16:39Z

Ready for another round of review.
The merge conflicts are minimal, I'll do it before merging to not mess with the current reviews and comments.

lookout-staging · 2018-12-05T16:22:00Z

cmd/lookoutd/common.go

-	Provider    string `long:"provider" choice:"github" choice:"json" default:"github" env:"LOOKOUT_PROVIDER" description:"provider name: github, json"`
-	ProbesAddr  string `long:"probes-addr" default:"0.0.0.0:8090" env:"LOOKOUT_PROBES_ADDRESS" description:"TCP address to bind the health probe endpoints"`
+	gocli.LogOptions `group:"Log Options"`
+	ConfigFile       string `long:"config" short:"c" default:"config.yml" env:"LOOKOUT_CONFIG_FILE" description:"path to configuration file"`


line is 138 characters (lll)

If you have feedback about this comment, please, tell us.

lookout-staging · 2018-12-05T16:22:00Z

cmd/lookoutd/common.go

+	ConfigFile       string `long:"config" short:"c" default:"config.yml" env:"LOOKOUT_CONFIG_FILE" description:"path to configuration file"`
+	GithubUser       string `long:"github-user" env:"GITHUB_USER" description:"user for the GitHub API"`
+	GithubToken      string `long:"github-token" env:"GITHUB_TOKEN" description:"access token for the GitHub API"`
+	Provider         string `long:"provider" choice:"github" choice:"json" default:"github" env:"LOOKOUT_PROVIDER" description:"provider name: github, json"`


line is 154 characters (lll)

If you have feedback about this comment, please, tell us.

lookout-staging · 2018-12-05T16:22:01Z

cmd/lookoutd/common.go

+	GithubUser       string `long:"github-user" env:"GITHUB_USER" description:"user for the GitHub API"`
+	GithubToken      string `long:"github-token" env:"GITHUB_TOKEN" description:"access token for the GitHub API"`
+	Provider         string `long:"provider" choice:"github" choice:"json" default:"github" env:"LOOKOUT_PROVIDER" description:"provider name: github, json"`
+	ProbesAddr       string `long:"probes-addr" default:"0.0.0.0:8090" env:"LOOKOUT_PROBES_ADDRESS" description:"TCP address to bind the health probe endpoints"`


line is 158 characters (lll)

If you have feedback about this comment, please, tell us.

smacker · 2018-12-05T16:40:19Z

Sorry most probably I did something wrong and github doesn't show context for my comments anymore...

carlosms · 2018-12-05T18:52:33Z

Besides the changes requested I've pushed another commit c9dbaa9 to make our LogOptions behave like go-cli's LogOption.

I didn't do the same for QueueOptions and DBOptions because they connect to other components, and it feels like this does not belong to an "initialization" phase. It's more about the code structure, it would work the same in the command's Init and Execute.

lookout-staging · 2018-12-05T18:59:26Z

cmd/lookout-sdk/push.go

 }

 type PushCommand struct {
+	gocli.PlainCommand `name:"push" short-description:"trigger a push event" long-description:"Provides a simple data server and triggers an analyzer push event"`


line is 159 characters (lll)

If you have feedback about this comment, please, tell us.

lookout-staging · 2018-12-05T18:59:26Z

cmd/lookout-sdk/review.go

 }

 type ReviewCommand struct {
+	gocli.PlainCommand `name:"review" short-description:"trigger a review event" long-description:"Provides a simple data server and triggers an analyzer review event"`


line is 165 characters (lll)

If you have feedback about this comment, please, tell us.

smacker · 2018-12-05T20:05:17Z

cmd/lookoutd/common.go

+
+	var err error
+	c.conf, err = c.initConfig()
+	if err != nil {


nit pick:

var err error c.conf, err = c.initConfig() return err

sorry 😅

Done in 63ae46e

smacker

awesome work! 👍 👍 👍

there is a tiny nitpick left

Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>

carlosms · 2018-12-07T10:48:42Z

Tests are failing because of #391

Most probably related to src-d/lookout#378

carlosms added the enhancement New feature or request label Dec 3, 2018

carlosms requested review from smacker and dpordomingo December 3, 2018 17:01

lookout-staging bot reviewed Dec 3, 2018

View reviewed changes

dpordomingo approved these changes Dec 4, 2018

View reviewed changes

smacker reviewed Dec 4, 2018

View reviewed changes

lookout-staging bot reviewed Dec 5, 2018

View reviewed changes

smacker reviewed Dec 5, 2018

View reviewed changes

smacker approved these changes Dec 5, 2018

View reviewed changes

carlosms added 2 commits December 7, 2018 09:40

Add go-cli dependency

8dafffd

Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>

Use go-cli library

5fffe2b

Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>

carlosms force-pushed the go-cli branch from 63ae46e to 5fffe2b Compare December 7, 2018 09:51

carlosms merged commit a20cace into src-d:master Dec 7, 2018

bzz added a commit to src-d/lookout-gometalint-analyzer that referenced this pull request Dec 11, 2018

ci: fix integration test

f7b9e50

Most probably related to src-d/lookout#378

bzz mentioned this pull request Dec 11, 2018

ci: fix integration test src-d/lookout-gometalint-analyzer#20

Merged

		)

		var app = gocli.New("dummy", version, build, "Dummy analyzer for testing")

Use go-cli lib #378

Use go-cli lib #378

Uh oh!

Conversation

carlosms commented Dec 3, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

smacker left a comment

Choose a reason for hiding this comment

Uh oh!

carlosms commented Dec 5, 2018

Uh oh!

lookout-staging bot Dec 5, 2018

Choose a reason for hiding this comment

Uh oh!

lookout-staging bot Dec 5, 2018

Choose a reason for hiding this comment

Uh oh!

lookout-staging bot Dec 5, 2018

Choose a reason for hiding this comment

Uh oh!

smacker commented Dec 5, 2018

Uh oh!

carlosms commented Dec 5, 2018

Uh oh!

lookout-staging bot Dec 5, 2018

Choose a reason for hiding this comment

Uh oh!

lookout-staging bot Dec 5, 2018

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!

smacker left a comment

Choose a reason for hiding this comment

Uh oh!

carlosms commented Dec 7, 2018

Uh oh!

Uh oh!