Skip to content

Conversation

@grayside
Copy link
Contributor

@grayside grayside commented Feb 1, 2018

This is a first take, I think there's some language and formatting tightening potential, but it all seems to work as you'd expect.

@grayside grayside requested review from febbraro and tekante February 1, 2018 02:44
}

cmd.out.Info("Starting Unison container")
cmd.out.Info("Sync volume '%s' created", volumeName)
Copy link
Member

Choose a reason for hiding this comment

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

I never see this message, even recording and playing in slow motion.

Copy link
Member

Choose a reason for hiding this comment

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

On further testing, I don't see it without verbose mode but I do see it in verbose mode.

// which is how we have configured dnsdock to provide names for containers.
func (cmd *ProjectSync) WaitForUnisonContainer(containerName string, timeoutSeconds int) (string, error) {
cmd.out.Info("Waiting for container to start")
cmd.out.SpinWithVerbose("Sync container '%s' started , waiting for unison server process...", containerName)
Copy link
Member

Choose a reason for hiding this comment

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

I never see this message.

Copy link
Member

Choose a reason for hiding this comment

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

Ignore this, I wasn't supposed to because I was running in non verbose mode. It's there in verbose mode.

@tekante
Copy link
Member

tekante commented Feb 2, 2018

I dropped a few notes about where I don't see messages though I consider that not particularly blocking.

Here is the sequence I see:
Spinner: Starting Outrigger Filesync (unison)
Checkmark: Starting sync volume: nwpl-sync (overwrites prev spinner)
Spinner: Starting sync container: nwpl-sync (same name)
Checkmark: Sync container 'nwpl-sync' started (overwrites prev spinner)
Spinner: Initializing file sync...
Checkmark: Initial sync detected (overwrites prev spinner)
Spinner: Waiting for initial sync to finish
Checkmark: File sync completed (overwrites prev spinner)

@tekante
Copy link
Member

tekante commented Feb 2, 2018

One other thing I noticed (which I also don't think needs to block) is the error message doesn't work as nicely as the success messages do. Since the message is getting returned however it doesn't seem to be as simple as swapping it to a logger message. Perhaps doing a log message and then returning the error?

rig-project-sync-error

@tekante
Copy link
Member

tekante commented Feb 2, 2018

Adjusted error message to work better with spinner

rig-project-sync-error-split

Copy link
Member

@tekante tekante left a comment

Choose a reason for hiding this comment

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

I think this is fine even if there are some quirks keeping a message or two from displaying for me

@febbraro febbraro merged commit 6e7566f into develop Feb 28, 2018
@febbraro febbraro deleted the sync-spinner branch February 28, 2018 17:33
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.

4 participants