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

MYST-188 / CLI Client #92

Merged
merged 17 commits into from Jan 19, 2018

Conversation

Projects
None yet
5 participants
@ignasbernotas
Copy link
Member

commented Jan 11, 2018

No description provided.

@@ -124,7 +124,7 @@ func validateRegistrationRequest(regReq identityRegistrationDto) (err error) {
func validateCreationRequest(createReq *identityCreationDto) (errors *validation.FieldErrorMap) {
errors = validation.NewErrorMap()
if createReq.Password == nil {
errors.ForField("password").AddError("required", "Field is required")
//errors.ForField("password").AddError("required", "Field is required")

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 11, 2018

Member

Why this line is commented out?

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Jan 12, 2018

Author Member

This should be disabled for pre-alpha.

This comment has been minimized.

Copy link
@donce

donce Jan 12, 2018

Contributor

This shouldn't be commented-out, because it already allows passing empty passwords. It just checks that password param was passed.
This allows {"password": ""}, but forbids {}.

func NewTequilaClient() *TequilaClient {
return &TequilaClient{
http: NewHttpClient(
"http://127.0.0.1:4050",

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 16, 2018

Member
  • Would be good to read inject from CLI arguments
  • Also sync with Tequilapi is startup values

This comment has been minimized.

Copy link
@zolia

zolia Jan 16, 2018

Member

you can use -tequilapi.address and -tequilapi.port which are implemented already as client arguments.

http HttpClientInterface
}

type StatusDto struct {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 16, 2018

Member

Why this only-one DTO is public?

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 17, 2018

Member

Fixed

@@ -0,0 +1,221 @@
package cli

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 16, 2018

Member

I would not create separate package, but add next to cmd/mysterium_client/command_interactive

completer := getAutocompleterMenu(c.TequilaClient)

rl, err := readline.NewEx(&readline.Config{
Prompt: "\033[31m»\033[0m ",

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 16, 2018

Member

Colors could be readable constants


rl, err := readline.NewEx(&readline.Config{
Prompt: "\033[31m»\033[0m ",
HistoryFile: "/tmp/mysterium-cli.log",

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 16, 2018

Member
  1. Would be good to use options.DirectoryRun
  2. Dont use / but filepath.Join() to cover different OSes
@@ -73,6 +82,11 @@ func (cmd *CommandRun) Run() error {
}

fmt.Printf("Api started on: %d\n", port)

if cmd.cli != nil {
cmd.cli.Run()

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 16, 2018

Member

Errors unhandled

connectionManager,
httpApiServer,
nil,

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 16, 2018

Member

Weird. You could construct 2 different commands depending on options:

  • command_run -
  • command_interactive - which starts command_run thru composition

@Waldz Waldz force-pushed the MYST-188-cli-tequilla-client branch from aec079c to 7aa81da Jan 17, 2018

func info(items ...interface{}) {
fmt.Printf(infoColor + "[INFO] \033[0m")
fmt.Println(items...)
}

This comment has been minimized.

Copy link
@donce

donce Jan 18, 2018

Contributor

There is a lot of boiler-plate duplication in these methods, maybe it's worth to extract common message pattern?

func log(statusColor, label string, items ...interfaceP{) {
   fmt.Printf("%s[%s] \033[0m", statusColor, label)
   fmt.Println(items...)
}

func warn(items ...interface{}) {
    log(warningColor, "WARNING", items)
}
func success(items ...interface{}) {
   ....
}

...
}

err = parseResponseError(response)
if err != nil {

This comment has been minimized.

Copy link
@donce

donce Jan 18, 2018

Contributor

Such conditional can be put into single line:

if err = parseResponseError(response); err != nil {
   ...
}
func parseResponseError(response *http.Response) error {
if response.StatusCode < 200 || response.StatusCode >= 300 {
return errors.New(fmt.Sprintf("Server response invalid: %s (%s)", response.Status, response.Request.URL))
}

This comment has been minimized.

Copy link
@donce

donce Jan 18, 2018

Contributor

This method is duplicated - it's in 3 places (client_rest.go, http_client.go, response.go). Maybe it's possible to re-use them somehow?

@donce

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2018

Not a single unit-test? :(

@@ -10,6 +10,7 @@ type CommandOptions struct {
DirectoryKeystore string
TequilaApiAddress string
TequilaApiPort int
InteractiveCli bool

This comment has been minimized.

Copy link
@donce

donce Jan 18, 2018

Contributor

Shouldn't this be InteractiveCLI according to go naming convention?

@Waldz

Waldz approved these changes Jan 19, 2018

Fixed

@Waldz Waldz merged commit 64b09f1 into master Jan 19, 2018

@Waldz Waldz deleted the MYST-188-cli-tequilla-client branch Jan 19, 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.