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

make --config a global option on rita command #631

Merged
merged 6 commits into from
Apr 24, 2021

Conversation

vivek-26
Copy link
Contributor

This PR does the following:

  • makes --config a global option on the rita command.
  • removes --config flag from subcommands (import, html-report etc.)
  • uses GlobalString to fetch config value from global state. For reference, see this issue.

Closes #595

@ethack
Copy link
Collaborator

ethack commented Mar 30, 2021

This is great, thank you!

Do you think you could also migrate the test-config command to use the global option?

$ ./rita test-config --help                            
NAME:
   rita test-config - Check the configuration file for validity

USAGE:
   rita test-config [command options] [arguments...]

OPTIONS:
   --config value, -c value  specify a config file to be used

With these changes the config flag isn't recognized after the command. This invocation is what was needed prior to these changes and now produces an error. This could be a breaking change for some users.

./rita show-beacons --config etc/rita.yaml test
Incorrect Usage: flag provided but not defined: -config

Do you know if there's a urlfave/cli option to have the --config flag recognized wherever it is placed?

If there's not a simpler solution than putting the config flag back into all the subcommands and testing each case for both global and local config flags then I would rather we just announce the breaking change and possibly hold this until a major version bump. But if there's a clean fix that lets the flag be recognized anywhere then I'd be up for that to maintain backwards compatibility.

@vivek-26
Copy link
Contributor Author

vivek-26 commented Mar 30, 2021

Do you think you could also migrate the test-config command to use the global option?

Sure, will do that.

Do you know if there's a urlfave/cli option to have the --config flag recognized wherever it is placed?

@ethack As per this issue, there are 2 ways to achieve this. We could do something like this or we can upgrade urfave/cli to v2. I wrote a very simple example using urfave/cli-v2 and global flag is recognised by all subcommands. However, upgrading urfave/cli to v2 might cause breaking changes. (example)

@vivek-26
Copy link
Contributor Author

vivek-26 commented Apr 3, 2021

@ethack I have updated PR to make --config flag available to both root command and all subcommands.

Current CLI behaviour is as follows -


Command: $: ./rita --config /etc/rita.yaml test-config
Behaviour: test-config subcommand is aware of the --config flag value passed to the root command.


Command: $: ./rita test-config --config /etc/rita.yaml
Behaviour: test-config subcommand uses --config flag passed to it.


Commands:
alias rita1=rita --config /etc/rita/config.yaml
$: rita1 test-config --config /etc/new-config.yaml

Behaviour: test-config subcommand overwrites --config value set globally - /etc/rita/config.yaml and uses /etc/new-config.yaml.


Closes #595

@vivek-26
Copy link
Contributor Author

@ethack PTAL

Copy link
Collaborator

@ethack ethack left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution!

@ethack ethack merged commit 3fef216 into activecm:master Apr 24, 2021
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.

Move --config to a global option
2 participants