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 exception when set-severity receives invalid logging level #3353

Merged
merged 2 commits into from
Sep 9, 2019

Conversation

kinow
Copy link
Member

@kinow kinow commented Sep 8, 2019

This is a small change with no associated Issue.

Executing something like cylc set-verbosity five ubirajara, you will get:

Traceback (most recent call last):
  File "/home/kinow/Development/python/workspace/cylc-flow/bin/cylc-set-verbosity", line 63, in main
    severity = LOGGING_LVL_OF[severity_str]
KeyError: 'ubirajara'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/kinow/Development/python/workspace/cylc-flow/venv/bin/cylc-set-verbosity", line 7, in <module>
    exec(compile(f.read(), __file__, 'exec'))
  File "/home/kinow/Development/python/workspace/cylc-flow/bin/cylc-set-verbosity", line 77, in <module>
    main()
  File "/home/kinow/Development/python/workspace/cylc-flow/cylc/flow/terminal.py", line 137, in wrapper
    wrapped_function(*wrapped_args, **wrapped_kwargs)
  File "/home/kinow/Development/python/workspace/cylc-flow/bin/cylc-set-verbosity", line 65, in main
    parser.error("Illegal logging level, %s" % severity)
UnboundLocalError: local variable 'severity' referenced before assignment

I believe that is because we are trying to log the severity value, while the invalid logging level provided by the user was actually in severity_str.

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Does not need tests (why? this part of the code is not really tested for now, except for functional tests. I think we don't need one for this small change.).
  • No change log entry required (why? e.g. invisible to users).
  • No documentation update required.

@kinow kinow self-assigned this Sep 8, 2019
@kinow kinow changed the title Fix UnboundLocalError for severity variable Fix exception when set-severity receives invalid logging level Sep 8, 2019
@kinow
Copy link
Member Author

kinow commented Sep 8, 2019

After this PR, the output changes to:

Usage: cylc [control] set-verbosity [OPTIONS] REG LEVEL 

Change the logging severity level of a running suite.  Only messages at
or above the chosen severity level will be logged; for example, if you
choose WARNING, only warnings and critical messages will be logged.

Arguments:
   REG                 Suite name
   LEVEL               INFO, NORMAL, WARNING, ERROR, CRITICAL, DEBUG

cylc-set-verbosity: error: Illegal logging level, ubirajara

@kinow kinow added the bug Something is wrong :( label Sep 8, 2019
@kinow kinow added this to the cylc-8.0.0 milestone Sep 8, 2019
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

👍

@matthewrmshin
Copy link
Contributor

Can merge after Travis CI passes.

@matthewrmshin matthewrmshin modified the milestones: cylc-8.0.0, cylc-8.0a1 Sep 9, 2019
@matthewrmshin
Copy link
Contributor

CodeCov not happy because this change is lacking a test (and actually never tested!) Do you want to add one?

@kinow
Copy link
Member Author

kinow commented Sep 9, 2019

CodeCov not happy because this change is lacking a test (and actually never tested!) Do you want to add one?

Shouldn't be too hard. And I was going to add one but

Does not need tests (why? this part of the code is not really tested for now, except for functional tests. I think we don't need one for this small change.).

I was not sure if it would survive #2802 . Said that, I started the IDE to write a quick unit test, but turns out it's not that simple. The tests in cylc/flow/tests/ have no knowledge of the scripts in ./bin. So we cannot import that code (another good reason for #2802 IMHO).

I think the only option is a functional test in the ./tests/ directory 😞 will take me a bit more as I am not quite fluent in shell, so will update PR until Friday 👍

@matthewrmshin
Copy link
Contributor

I'll write a test for you and raise a PR on your branch.

@kinow
Copy link
Member Author

kinow commented Sep 9, 2019

I'll write a test for you and raise a PR on your branch.

🙏 thank you!!!

@matthewrmshin
Copy link
Contributor

See kinow#5.

@kinow
Copy link
Member Author

kinow commented Sep 9, 2019

Merged! Thanks a lot @matthewrmshin ! Now up to Travis

@matthewrmshin matthewrmshin merged commit fa97ab0 into cylc:master Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants