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

Display an error/warning when mutually exclusive flags are set in scope #611

Closed
seanvaleo opened this issue Oct 15, 2021 · 4 comments · Fixed by #821 or #833
Closed

Display an error/warning when mutually exclusive flags are set in scope #611

seanvaleo opened this issue Oct 15, 2021 · 4 comments · Fixed by #821 or #833
Assignees
Labels
cli good first issue Good issue for newcomers

Comments

@seanvaleo
Copy link
Collaborator

seanvaleo commented Oct 15, 2021

These combinations of flags should probably throw a user error or warning message:

(with scope run):

  • --cribldest && --eventdest
  • --cribldest && --metricsdest
  • --userconfig && (--metricsdest || --eventsdest || --cribldest || --loglevel ....etc)
  • --passthrough && [anything else]

Considerations
Currently scope proceeds for all of these combinations, silently overriding or bypassing mutually exclusive flags. Should we warn and let scope proceed anyway? Should we throw an error and have the user fix their arguments?

Related: spf13/cobra#1216

@seanvaleo
Copy link
Collaborator Author

seanvaleo commented Mar 4, 2022

It makes more sense to notify the user of error, to avoid unexpected results; and to avoid discovering a bug.

In cobra there is an option to create flag sets but this appears to have limitations. It is easier to write conditions that check for invalid combinations, and return an error to the user.

We should:

  • Create an arg matrix at the top of command files
  • Create conditions that return an error with bad argument combinations

@seanvaleo
Copy link
Collaborator Author

seanvaleo commented Mar 4, 2022

  • scope attach
  • scope completion
  • scope dash
  • scope events
  • scope flows
  • scope help
  • scope history
  • scope metrics
  • scope prune
  • scope ps
  • scope run

@seanvaleo
Copy link
Collaborator Author

seanvaleo commented Mar 7, 2022

QA INSTRUCTIONS

Part 1
Do:

docker run --rm -it cribl/scope:<VERSION_ID>
apt update
apt install curl -y
scope curl wttr.in
scope events --follow --sort _time
scope events --match http --all
scope flows --in --out
# etc...

Expect:
Nonsensical argument combinations should not be allowed and should exit and throw a user help message.

@jrcheli
Copy link
Contributor

jrcheli commented Mar 10, 2022

I found one small thing during the bugathon of this with 1.0.2-rc0. I'll write it up in #839. In all other ways, this looks great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli good first issue Good issue for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants