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

Log view: display any errors in listing the log files #1842

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Jun 24, 2024

If there is a failure to list the log files, the user should be presented with an error message, not left in the dark.

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).
  • No tests needed
  • No changelog needed

@MetRonnie

This comment was marked as resolved.

@MetRonnie MetRonnie closed this Jul 10, 2024
@MetRonnie MetRonnie deleted the cat-log branch July 10, 2024 11:58
@MetRonnie MetRonnie restored the cat-log branch July 10, 2024 12:04
@MetRonnie MetRonnie reopened this Jul 10, 2024
@MetRonnie MetRonnie marked this pull request as ready for review July 10, 2024 12:04
@MetRonnie
Copy link
Member Author

Actually this change still makes sense irrespective of cylc/cylc-uiserver#607

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

This doesn't appear to have made any functional difference in my tests:

test 1:

diff --git a/cylc/flow/scripts/cat_log.py b/cylc/flow/scripts/cat_log.py
index e53524cc6..78d1b3b25 100755
--- a/cylc/flow/scripts/cat_log.py
+++ b/cylc/flow/scripts/cat_log.py
@@ -452,6 +452,9 @@ def _main(
     except KeyError:
         mode = options.mode
 
+    # if mode != 'list-dir':
+    sys.exit('foo bar baz')
+
     if tokens and tokens.get('cycle') and not tokens.get('task'):
         print('Please provide a workflow, task or job ID', file=sys.stderr)
         sys.exit(1)

test 2:

diff --git a/cylc/flow/scripts/cat_log.py b/cylc/flow/scripts/cat_log.py
index e53524cc6..17baa6f35 100755
--- a/cylc/flow/scripts/cat_log.py
+++ b/cylc/flow/scripts/cat_log.py
@@ -452,6 +452,9 @@ def _main(
     except KeyError:
         mode = options.mode
 
+    if mode != 'list-dir':
+        sys.exit('foo bar baz')
+
     if tokens and tokens.get('cycle') and not tokens.get('task'):
         print('Please provide a workflow, task or job ID', file=sys.stderr)
         sys.exit(1)

Before (test1: test2):

Screenshot from 2024-08-12 10-08-16

After (test1: test2):

Screenshot from 2024-08-12 10-08-00

@MetRonnie
Copy link
Member Author

Try

    if mode == 'list-dir':
        sys.exit('foo bar baz')

@oliver-sanders
Copy link
Member

oliver-sanders commented Aug 12, 2024

I did, see my first diff.

I can get the list-dir command to fail (with an error logged at the UIS) but no notification of that error in the UI.

@MetRonnie
Copy link
Member Author

MetRonnie commented Aug 13, 2024

Ah sorry, this only applies to network/graphql errors. E.g. try removing the cat_log_files function in cylc/uiserver/resolvers.py: https://github.com/cylc/cylc-uiserver/blob/dfb1ab41c1284444e37170dd50abd76849469459/cylc/uiserver/resolvers.py#L433

@oliver-sanders
Copy link
Member

Removing the cat_log_files function will create an internal code error not a network error right? Note GraphQL errors are another form of internal error.

This PR does not appear to assist with diagnosing remote errors, e.g. try creating an SSH error with your config.

This diff introduces a common network error (host not found), but no error is reported in the log view:

diff --git a/cylc/flow/remote.py b/cylc/flow/remote.py
index e0ab7544a..e99903e0a 100644
--- a/cylc/flow/remote.py
+++ b/cylc/flow/remote.py
@@ -289,7 +289,8 @@ def construct_ssh_cmd(
     if stdin is None:
         command.append('-n')
 
-    command.append(host)
+    # command.append(host)
+    command.append('elephant')
 
     # Pass CYLC_VERSION and optionally, CYLC_CONF_PATH & CYLC_UTC through.
     command += ['env', quote(r'CYLC_VERSION=%s' % CYLC_VERSION)]

I think this PR may make sense, but at present I don't think it is helping as we need the UIS to capture and forward these errors which is clearly isn't doing at present. We should get the errors into the schema at the UIS end first, then make sure the log view is displaying them after.

@MetRonnie
Copy link
Member Author

MetRonnie commented Aug 14, 2024

At the moment this PR guards prevents any bug in the UIS code being silent.

We agreed it doesn't make sense to show an error snackbar for non-zero ret code of cat-log in cylc/cylc-uiserver#607:

I realised that there are a few ways cat-log will exit non-zero when navigating in the UI (e.g. navigating to /log/non-exist), so have settled for just logging the stderr and returning an empty list as before

Maybe we should ensure ret code 1 means no files found, and show an error snackbar for any ret code > 1?

@oliver-sanders oliver-sanders modified the milestones: 2.6.0, 2.7.0 Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants