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.

Sign up for GitHub

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

Jump to bottom

[JUJU-1700] Added --watch flag to watch status changes via Viddy tool #14528

Merged
merged 6 commits into from
Sep 26, 2022
Merged

[JUJU-1700] Added --watch flag to watch status changes via Viddy tool #14528

merged 6 commits into from
Sep 26, 2022

Conversation

anvial
Copy link
Member

@anvial anvial commented Aug 29, 2022 •
edited
Loading

This PR adds Viddy tool usage to juju status --watch command. This flag allows watching juju status changes via the Viddy (https://github.com/juju/viddy) tool.

Checklist

  • Code style: imports ordered, good names, simple structure, etc
  • Comments saying why design decisions were made
  • Go unit tests, with comments saying what you're testing
  • Integration tests, with comments saying what you're testing

QA steps

juju bootstrap lxd lxd
juju deploy tiny-bash

juju status --watch 1s
juju status --watch 5s
juju status --watch 1s --no-color

@jnsgruk
Copy link
Member

jnsgruk commented Aug 29, 2022

Feels like doing both is just bloat - can we just make viddy the default, or is it dependant on an external binary?

@anvial
Copy link
Member Author

anvial commented Aug 29, 2022 •
edited
Loading

Feels like doing both is just bloat - can we just make viddy the default, or is it dependant on an external binary?

No, there is no problem with doing viddy the default.
We use our viddy fork that provides Viddy as an importable package (so there is no dependency on an external binary).

@anvial anvial changed the title [JUJU-1700] Added --viddy flag to watch status changes via Viddy tool [JUJU-1700] Added --watch flag to watch status changes via Viddy tool Aug 29, 2022
@anvial
Copy link
Member Author

anvial commented Aug 29, 2022

Now Viddy is the default for juju status --watch

@anvial anvial mentioned this pull request Aug 30, 2022
Closed
manadart
manadart requested changes Sep 20, 2022
View reviewed changes
Copy link
Member

@manadart manadart left a comment

Choose a reason for hiding this comment

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

Some minor things.

cmd/juju/status/status.go Outdated
@@ -75,6 +75,9 @@ type statusCommand struct {

// watch indicates the time to wait between consecutive status queries
watch time.Duration

// viddy indicates the time to wait between consecutive status queries (with viddy tool)
viddy time.Duration
Copy link
Member

Choose a reason for hiding this comment

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

This is not used and needs deleting.

cmd/juju/status/status.go Outdated

// repeat the call using a ticker
ticker := time.NewTicker(c.watch)
// Prepare Viddy args
Copy link
Member

Choose a reason for hiding this comment

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

This comment is not required.

cmd/juju/status/status.go Outdated
fmt.Printf("\u001Bc")
// statusArgsWithoutWatchFlag returns all args cut off '--watch' flag of status commands
// and the value of '--watch' flag
func (c *statusCommand) statusArgsWithoutWatchFlag(args []string) ([]string, string) {
Copy link
Member

Choose a reason for hiding this comment

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

You already have the watch value in c.watch no need to return it. You only need the stripped args.

@manadart
Copy link
Member

After running this, I have a weird issue with uxterm. My cursor has disappeared...

@manadart manadart force-pushed the JUJU-1700-add-viddy-flag-to-juju-status-command branch from 6d4dc90 to c5edf70 Compare September 20, 2022 11:31
@anvial
Copy link
Member Author

anvial commented Sep 23, 2022

/merge

1 similar comment
@anvial
Copy link
Member Author

anvial commented Sep 26, 2022

/merge

@anvial
Copy link
Member Author

anvial commented Sep 26, 2022

/merge

@jujubot jujubot merged commit f982300 into juju:develop Sep 26, 2022
@manadart manadart mentioned this pull request Oct 10, 2022
Merged
@anvial anvial deleted the JUJU-1700-add-viddy-flag-to-juju-status-command branch November 2, 2022 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manadart manadart manadart approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@anvial @jnsgruk @manadart @hpidcock @jujubot