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

Fix parsing of watch flag on status command #16445

Merged
merged 1 commit into from
Oct 17, 2023
Merged

Conversation

nvinuesa
Copy link
Member

On the status command, when passing the --watch flag, we pass the entire command line arguments (including 'juju status') along with all the flags except --watch to viddy, which will run the command indefinitely with an interval.

To do this, we must remove the --watch flag along with its value from the list of OS arguments to be passed to viddy. This was incorrectly done because it only accepted unix-style flags.

The fix is to remove both unix and gnu style when passing the watch flag.

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
  • doc.go added or updated in changed packages

QA steps

After bootstrapping and adding a model, get the status with --watch on two different styles:

juju status --watch 1s --relations
juju status --watch=1s --relations  

also,

go test github.com/juju/juju/cmd/juju/status/... -gocheck.v

Links

Launchpad bug: https://bugs.launchpad.net/juju/+bug/2037914

Jira card: JUJU-4782

@nvinuesa nvinuesa added bug The PR addresses a bug 3.1 labels Oct 17, 2023
Copy link
Member

@SimonRichardson SimonRichardson left a comment

Choose a reason for hiding this comment

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

Can you check that -w=1s still works?

@nvinuesa
Copy link
Member Author

@SimonRichardson has the -w flag ever worked? I'm testing on 3.1 and it doesn't

@jack-w-shaw
Copy link
Member

I'm not aware of -w being supported either. Perhaps in 2.9?

Copy link
Member

@jack-w-shaw jack-w-shaw left a comment

Choose a reason for hiding this comment

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

LGTM

break
}
}

if !c.noColor {
jujuStatusArgsWithoutWatchFlag = append(jujuStatusArgsWithoutWatchFlag, "--color")
}
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 a side effect. I'm guessing the reason we have this here is to ensure we pass --color to ensure color is explict for viddy to avoid bugs? The fact it isn't obvious is a smell

Perhaps we should rename the function to statusCommandForViddy or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@SimonRichardson
Copy link
Member

We definitely do support single character shortcut, so that also needs fixing

@nvinuesa
Copy link
Member Author

@SimonRichardson we do support single char shortcut but only for some of the flags:

Summary:
Report the status of the model, its machines, applications and units.

Global Options:
--debug  (= false)
    Equivalent to --show-log --logging-config=<root>=DEBUG
-h, --help  (= false)
    Show help on a command or other topic.
--logging-config (= "")
    Specify log levels for modules
--quiet  (= false)
    Show no informational output
--show-log  (= false)
    If set, write the log file to stderr
--verbose  (= false)
    Show more verbose output

Command Options:
-B, --no-browser-login  (= false)
    Do not use web browser for authentication
--color  (= false)
    Use ANSI color codes in tabular output
--format  (= tabular)
    Specify output format (json|line|oneline|short|summary|tabular|yaml)
--integrations  (= false)
    Show 'integrations' section in tabular output
-m, --model (= "")
    Model to operate in. Accepts [<controller name>:]<model name>|<model UUID>
--no-color  (= false)
    Disable ANSI color codes in tabular output
-o, --output (= "")
    Specify an output file
--relations  (= false)
    The same as '--integrations'
--retry-count  (= 3)
    Number of times to retry API failures
--retry-delay  (= 100ms)
    Time to wait between retry attempts
--storage  (= false)
    Show 'storage' section in tabular output
--utc  (= false)
    Display timestamps in the UTC timezone
--watch  (= 0s)
    Watch the status every period of time

and watch is not one of them

On the status command, when passing the --watch flag, we pass the entire
command line arguments (including 'juju status') along with all the
flags except --watch to viddy, which will run the command indefinitely
with an interval.

To do this, we must remove the --watch flag along with its value from
the list of OS arguments to be passed to viddy. This was incorrectly
done because it only accepted unix-style flags.

The fix is to remove both unix and gnu style when passing the watch
flag.
@nvinuesa
Copy link
Member Author

/merge

@jujubot jujubot merged commit 6dd3be0 into juju:3.1 Oct 17, 2023
20 of 21 checks passed
@manadart manadart mentioned this pull request Oct 18, 2023
jujubot added a commit that referenced this pull request Oct 18, 2023
#16455

Merge from 3.1 to bring forward:
- #16453 from manadart/3.1-modelcache-add-machine-test
- #16452 from manadart/3.1-firewaller-CMR-consume-test
- #16445 from nvinuesa/juju-4782
- #16435 from hmlanigan/rename-uniter-corecharm-jujucharm

Trivial conflict in worker/modelcache/worker_test.go
@hpidcock hpidcock mentioned this pull request Oct 19, 2023
jujubot added a commit that referenced this pull request Oct 19, 2023
#16461

Forward ports:
- #16426
- #16435
- #16445
- #16452
- #16451
- #16453
- #16455
- #16456
- #16457
- #16460

Conflicts:
- go.mod
- worker/uniter/op_callbacks.go
- worker/uniter/uniter.go
- worker/uniter/util_test.go
@hpidcock hpidcock mentioned this pull request Oct 19, 2023
@ycliuhw ycliuhw mentioned this pull request Oct 19, 2023
jujubot added a commit that referenced this pull request Oct 20, 2023
#16462

Forward ports:
- #16426
- #16435
- #16445
- #16452
- #16451
- #16453
- #16455
- #16456
- #16457
- #16460
- #16461

Conflicts:
- apiserver/facades/agent/provisioner/imagemetadata_test.go
- environs/bootstrap/bootstrap.go
- environs/simplestreams/datasource.go
- environs/simplestreams/datasource_test.go
- go.sum
- worker/dbaccessor/package_test.go
- worker/firewaller/firewaller_test.go
- worker/modelcache/worker_test.go
- worker/uniter/charm/bundles_test.go
- worker/uniter/uniter_test.go
- worker/uniter/util_test.go
- cmd/juju/status/status_internal_test.go
jujubot added a commit that referenced this pull request Oct 23, 2023
#16470

Merge branch `3.3` into `main`:
- #16426
- #16435
- #16445
- #16452
- #16451
- #16453
- #16455
- #16456
- #16457
- #16460
- #16461
- #16454
- #16394
- #16469

```
# Conflicts:
# apiserver/common/secrets/access.go
# apiserver/common/secrets/access_test.go
# apiserver/common/secrets/drain.go
# apiserver/common/secrets/drain_test.go
# apiserver/common/secrets/mocks/commonsecrets_mock.go
# apiserver/common/secrets/watcher.go
# apiserver/common/secrets/watcher_test.go
# apiserver/facades/agent/provisioner/imagemetadata_test.go
# apiserver/facades/agent/secretsdrain/mocks/modelstate.go
# apiserver/facades/agent/secretsdrain/mocks/secretsprovider.go
# apiserver/facades/agent/secretsdrain/package_test.go
# apiserver/facades/agent/secretsdrain/register.go
# apiserver/facades/agent/secretsdrain/state.go
# cmd/jujud/agent/caasoperator/manifolds.go
# environs/bootstrap/bootstrap.go
# environs/simplestreams/datasource.go
# environs/simplestreams/datasource_test.go
# go.sum
# worker/dbaccessor/package_test.go
# worker/firewaller/firewaller_test.go
# worker/modelcache/worker_test.go
# worker/secretsdrainworker/manifold.go
# worker/secretsdrainworker/manifold_test.go
# worker/secretsdrainworker/package_test.go
# worker/uniter/charm/bundles_test.go
# worker/uniter/uniter_test.go
# worker/uniter/util_test.go
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.1 bug The PR addresses a bug
Projects
None yet
4 participants