Skip to content

Fix/cli logs#464

Merged
NicolasMahe merged 14 commits intodevfrom
fix/cli-logs
Sep 17, 2018
Merged

Fix/cli logs#464
NicolasMahe merged 14 commits intodevfrom
fix/cli-logs

Conversation

@krhubert
Copy link
Copy Markdown
Contributor

No description provided.

NicolasMahe
NicolasMahe previously approved these changes Sep 14, 2018
@NicolasMahe
Copy link
Copy Markdown
Member

@antho1404 can you review please

Comment thread commands/service_execute.go Outdated
return err

// XXX: double check if sleep before was too short.
case <-time.After(5 * time.Second):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we might have some long running tasks, for example submitting a bitcoin transaction will take few minutes if we wait for the confirmation.
If it's too long for users, they can exit.

Copy link
Copy Markdown
Contributor Author

@krhubert krhubert Sep 16, 2018

Choose a reason for hiding this comment

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

I discussed this with @NicolasMahe as well. The only solution for this problem is making sure stream reader is ready to use after the open. Otherwise, we can break after a long running task, or app can miss the results because we have no guarantee that 1 second sleep would be enough. Also, we need this thing working with network PoC. I thought about fixing it right after I get home.

Comment thread commands/service_logs.go Outdated
max := xstrings.FindLongest(dependencies)
for i, dep := range dependencies {
c := colors[i%len(colors)]
prefixes[dep] = c.Sprintf("% *s |", max, dep)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not valid because the display is different now. The dependency name is aligned to the right instead of the left.

screen shot 2018-09-16 at 4 13 42 pm

vs

screen shot 2018-09-16 at 4 13 33 pm

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

will do

@NicolasMahe NicolasMahe merged commit febe33e into dev Sep 17, 2018
@NicolasMahe NicolasMahe deleted the fix/cli-logs branch September 17, 2018 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants