Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
cat-log: list out/err files when available via tailer #6480
cat-log: list out/err files when available via tailer #6480
Changes from all commits
4714a46
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 599 in cylc/flow/scripts/cat_log.py
cylc/flow/scripts/cat_log.py#L599
Check warning on line 604 in cylc/flow/scripts/cat_log.py
cylc/flow/scripts/cat_log.py#L604
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to include the
proc.communicate()
in the suppressing of theKeyboardInterrupt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a
suppress
block, so the code execution will continue on ifKeyboardInterrupt
is caught leading to an error further on in the code.As it stands, if you ctrl+c here, you'll get a KeyboardInterrupt traceback:
But if we do the awaiting inside this block, it just turns into a different error:
I don't think kill signals are very well handled here at present, not really the fault of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the whole
if isinstance
block is included in thesuppress
block, it avoids traceback on ctrl+c. But not that important I guessThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The traceback is a cosmetic problem, but I think there are pre-existing functional issues here, #6609
Check warning on line 616 in cylc/flow/scripts/cat_log.py
cylc/flow/scripts/cat_log.py#L615-L616
Check warning on line 626 in cylc/flow/scripts/cat_log.py
cylc/flow/scripts/cat_log.py#L626
Check warning on line 631 in cylc/flow/scripts/cat_log.py
cylc/flow/scripts/cat_log.py#L631
Check warning on line 637 in cylc/flow/scripts/cat_log.py
cylc/flow/scripts/cat_log.py#L637
Check warning on line 642 in cylc/flow/scripts/cat_log.py
cylc/flow/scripts/cat_log.py#L639-L642