Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cli changes #421

Merged
merged 8 commits into from Oct 9, 2018

Conversation

Projects
None yet
4 participants
@soffokl
Copy link
Member

commented Oct 2, 2018

Closes #335
Closes #402

@soffokl soffokl added the enhancement label Oct 2, 2018

@soffokl soffokl self-assigned this Oct 2, 2018

@soffokl soffokl requested review from Waldz, zolia and vkuznecovas Oct 2, 2018

@soffokl soffokl requested a review from tadovas as a code owner Oct 2, 2018

// Command describes CLI based Mysterium UI
type Command struct {
// command describes CLI based Mysterium UI
type command struct {

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 2, 2018

Member

cliApplication maybe instead of 2 command structs

tequilapi: tequilapi_client.NewClient(nodeOptions.TequilapiAddress, nodeOptions.TequilapiPort),
}

cmd.RegisterSignalCallback(utils.HardKiller(cmdCLI.Kill))

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 2, 2018

Member

Should not we register signal handler for application not for each command?

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 2, 2018

Author Member

I think it's OK to have different channels and goroutines for a subcommand since we don't know on the application level what command will be started and what callback should be called.

@zolia
Copy link
Member

left a comment

lgtm

@soffokl soffokl dismissed stale reviews from zolia and vkuznecovas via 37e3d2a Oct 3, 2018

historyFile: filepath.Join(nodeOptions.Directories.Data, ".cli_history"),
tequilapi: tequilapi_client.NewClient(nodeOptions.TequilapiAddress, nodeOptions.TequilapiPort),
}
cmd.RegisterSignalCallback(utils.HardKiller(cmdCLI.Kill))

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 3, 2018

Member

Node should be stopped also

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 4, 2018

Author Member

Added stop of the Node to the After function.


errorChannel := make(chan error)
go func() {
if err := di.Node.Start(); err != nil {

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 3, 2018

Member

Node starting/stoping logic is duplicated now. Maybe we can wrap with like: NewDaemonCommand(command_cli.NewCommand()) * cli.Command

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 4, 2018

Author Member

Extracted daemon to the separate function.

@soffokl soffokl force-pushed the cli-changes branch from fe57deb to 6b02ba7 Oct 4, 2018

errorChannel := make(chan error, 1)
daemon.StartDaemon(ctx, &di, errorChannel)

di.BootstrapServiceComponents(cmd.ParseFlagsNode(ctx), service.Options{

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 5, 2018

Member

Parsing NodeOptions 2 times, can we inject

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 8, 2018

Author Member

Getting it from the di.NodeOptions second time.

return <-errorChannel
},
After: func(ctx *cli.Context) error {
err := di.Node.Kill()

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 5, 2018

Member

Dont we need to stop cli command?

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 5, 2018

Member

I would reuse same stop function as for After

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 8, 2018

Author Member

When the app finished it will be destroyed. There are no parts that require a graceful shutdown.

Show resolved Hide resolved cmd/commands/cli/command.go
}

return di.Node.Wait()
return <-errorChannel
},
After: func(ctx *cli.Context) error {
err := di.Node.Kill()

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 5, 2018

Member

di.Shutdown() should be done somewhere.
I suggest to pull di.Bootstrap to here also

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 5, 2018

Member

e.g. maybe we move/merge StartDaemon() helper to di.Bootstrap()

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 8, 2018

Author Member

Merged it to the di.Bootstrap()

return errorServiceManager
}
return errorNode
return errorServiceManager

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 5, 2018

Member

We need to stop all the dependencies. Currently Node is not stopped

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 8, 2018

Author Member

Added it to the di.Shutdown()

@soffokl soffokl force-pushed the cli-changes branch from 68a3c52 to 134fb71 Oct 8, 2018

@soffokl soffokl requested review from Waldz, vkuznecovas and zolia Oct 8, 2018

Show resolved Hide resolved cmd/di.go

soffokl added some commits Oct 2, 2018

soffokl added some commits Oct 8, 2018

@soffokl soffokl force-pushed the cli-changes branch from 134fb71 to 7e780af Oct 9, 2018

@Waldz

Waldz approved these changes Oct 9, 2018

@soffokl soffokl merged commit 522f795 into master Oct 9, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

soffokl added a commit that referenced this pull request Oct 9, 2018

soffokl added a commit that referenced this pull request Oct 9, 2018

@soffokl soffokl deleted the cli-changes branch Oct 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.