Conversation
cmd/dummy/main.go
Outdated
| build = "undefined" | ||
| ) | ||
|
|
||
| var app = gocli.New("dummy", version, build, "Dummy analyzer for testing") |
There was a problem hiding this comment.
gocli.New(name, version, build, "Dummy analyzer for testing")
util/cli/log.go
Outdated
|
|
||
| // 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 |
There was a problem hiding this comment.
it also applies the configuration to logrus.
cmd/lookoutd/serve.go
Outdated
| ctx, stopCtx := context.WithCancel(context.Background()) | ||
| stopCh := make(chan error, 1) | ||
|
|
||
| cli.InitLog(&c.LogOptions, name) |
There was a problem hiding this comment.
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
}
There was a problem hiding this comment.
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
cmd/lookoutd/serve.go
Outdated
| queueConsumerCommand | ||
| } | ||
|
|
||
| func (c *ServeCommand) Execute(args []string) error { |
There was a problem hiding this comment.
Let's use ExecuteContext and remove our custom signal handling.
smacker
left a comment
There was a problem hiding this comment.
There are few inline comments. But also maybe we should change our private init functions to use Initializer where it's appropriate.
|
Ready for another round of review. |
cmd/lookoutd/common.go
Outdated
| 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"` |
There was a problem hiding this comment.
line is 138 characters (lll)
If you have feedback about this comment, please, tell us.
cmd/lookoutd/common.go
Outdated
| 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"` |
There was a problem hiding this comment.
line is 154 characters (lll)
If you have feedback about this comment, please, tell us.
cmd/lookoutd/common.go
Outdated
| 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"` |
There was a problem hiding this comment.
line is 158 characters (lll)
If you have feedback about this comment, please, tell us.
|
Sorry most probably I did something wrong and github doesn't show context for my comments anymore... |
|
Besides the changes requested I've pushed another commit c9dbaa9 to make our I didn't do the same for |
| } | ||
|
|
||
| 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"` |
There was a problem hiding this comment.
line is 159 characters (lll)
If you have feedback about this comment, please, tell us.
| } | ||
|
|
||
| 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"` |
There was a problem hiding this comment.
line is 165 characters (lll)
If you have feedback about this comment, please, tell us.
cmd/lookoutd/common.go
Outdated
|
|
||
| var err error | ||
| c.conf, err = c.initConfig() | ||
| if err != nil { |
There was a problem hiding this comment.
nit pick:
var err error
c.conf, err = c.initConfig()
return errsorry 😅
smacker
left a comment
There was a problem hiding this comment.
awesome work! 👍 👍 👍
there is a tiny nitpick left
Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>
Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>
|
Tests are failing because of #391 |
Most probably related to src-d/lookout#378
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.