CLI: Add --verbose mode option #89
Conversation
openshift-ci-robot
added
the
size/S
MartinBasti
left a comment
MartinBasti
left a comment
There was a problem hiding this comment.
Current implementation doesn't support global options because only first arg is passed to main cli parser.
operatorcourier/cli.py
Outdated
| 'CRITICAL'], | ||
| default='WARNING', type=str.upper) | ||
|
|
||
| args = parser.parse_args(sys.argv[1:2]) |
There was a problem hiding this comment.
This PR is not working due this line. Because PR with argparse subparsers was rejected, I'm waiting for suggestion how to update this.
There was a problem hiding this comment.
Two comments here:
-
I'm still not sure I see why not using subparsers is preventing a top level flag like this. Why can't we just explicitly parse this command in this method? We aren't throwing anything away.
-
The PR that included a move to subparsers wasn't rejected because it used subparsers. It was rejected because the PR was attempting to address two unrelated things (adding subparsers and attempting to resolve this issue: document --ui_validate_io switch #77). I just stated that moving to subparsers didn't fix that issue, and either the PR should have been updated to fix that issue and explain why adding subparser use was needed to resolve it OR stop referencing that issue and instead just add subparsers and in a detailed way explain why that was useful. Instead of following up on my point, the PR was just closed.
I am not against using subparsers, I just believe that git history should be concise and clean and that every PR should have a singular stated purpose. If you strongly believe that the best way to implement this feature is to include the use of subparsers then feel free to either reopen the other PR with what I was requesting, or feel free to include that change in this PR if you feel it is necessary for this change. But when you do, please clearly state why that is the case.
There was a problem hiding this comment.
- main parser uses only first argument
sys.argv[1:2] - command parsers uses second+ argument for parsing
sys.argv[2:] - when I use
--log-levelon first position I got error that subcommand is required - when I use subcommand on first position, log-level is just ignored
- because log-level is optional, I cannot hardcode
sys.argv[3:] - I'm not gonna copy that option to each command parser
- subparsers handles this even without thinking about that some
sys.argv[]exists
- I will reopen it then
|
Reason for this PR was in #85 (comment) as it was not nice to find which part of code failed from CLI output |
operatorcourier/cli.py
Outdated
| action='version', version=__version__) | ||
| parser.add_argument('--log-level', dest='log_level', | ||
| help="Set logging level (default: WARNING)", | ||
| choices=['DEBUG', 'INFO', 'WARNING', 'ERROR', |
There was a problem hiding this comment.
Do we really want to allow all of these options? Why not just allow DEBUG and INFO? By default we will already log the others.
There was a problem hiding this comment.
What is wrong about having all of them? when I don't care about WARNINGs I can enable only errors.
I can use --verbose or --debug to enable DEBUG level, if you insist
There was a problem hiding this comment.
I would prefer -v and --verbose to enable DEBUG from the command line since this is a much more common convention.
There was a problem hiding this comment.
-v is already used
I will rework that to --verbose
MartinBasti
changed the title
Enables logging at DEBUG level * also show traceback as debug log Signed-off-by: Martin Bašti <mbasti@redhat.com>
|
Ready for review |
|
/lgtm |
|
Is there anything blocking this PR from merging? |
Was waiting on one more set of eyes. Thanks @JEREMYLINLIN |
4 participants
Signed-off-by: Martin Bašti mbasti@redhat.com