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.

Sign up for GitHub

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

Jump to bottom

tui: open log files in external application #6611

Merged
merged 5 commits into from
Mar 4, 2025
Merged

tui: open log files in external application #6611

merged 5 commits into from
Mar 4, 2025

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Feb 14, 2025 •
edited
Loading

Requested in: https://cylc.discourse.group/t/feature-request-cylc-tui-searching-job-out-job-err-files/1122/2

  • Allow log files to be open in external applications.
  • Tui will suspend whilst the external tool is open, and resume once it has closed.
  • Options implemented are $EDITOR, $GEDITOR, $PAGER and vim as a backup.

tui-external-editor

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).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included - very difficult to test
  • Changelog entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/797.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@oliver-sanders oliver-sanders self-assigned this Feb 14, 2025
@oliver-sanders oliver-sanders added this to the 8.5.0 milestone Feb 14, 2025
@oliver-sanders oliver-sanders force-pushed the tui-open-log-in-external-tool branch from 3a22611 to 4eb3f7d Compare February 14, 2025 15:42
@oliver-sanders oliver-sanders mentioned this pull request Feb 14, 2025
Merged
1 task
@oliver-sanders oliver-sanders force-pushed the tui-open-log-in-external-tool branch from 4eb3f7d to 853af33 Compare February 14, 2025 15:51
@oliver-sanders oliver-sanders marked this pull request as ready for review February 14, 2025 15:51
oliver-sanders
oliver-sanders commented Feb 14, 2025
View reviewed changes
cylc/flow/tui/overlay.py Outdated
Comment on lines 507 to 500
with tempfile.NamedTemporaryFile('w+') as temp_file:
# write the text into a temp file
temp_file.write(text)
temp_file.seek(0, 0)

# make the file readonly to avoid confusion
os.chmod(temp_file.name, stat.S_IRUSR)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the approach we used to use cylc view. Dump the text into a temp file and mark it as readonly so that people don't think they can edit it.

Note, we can't reliably edit the file locally because it might not be a local file.

@oliver-sanders oliver-sanders force-pushed the tui-open-log-in-external-tool branch from 853af33 to f0695a9 Compare February 14, 2025 16:12
hjoliver
hjoliver reviewed Feb 16, 2025
View reviewed changes
@hjoliver
Copy link
Member

For coverage, I think you could test this with a fake editor that just reads the file the in, then exits?

@oliver-sanders oliver-sanders force-pushed the tui-open-log-in-external-tool branch from f0695a9 to c51d584 Compare February 18, 2025 16:57
@oliver-sanders
tui: open log files in external application
38b4c09 
* Allow log files to be open in external applications.
* Tui will suspend whilst the external tool is open, and resume once it
  has closed.
* Options implemented are `$EDITOR`, `$GEDITOR`, `$PAGER` and `vim` as a
  backup.
@oliver-sanders oliver-sanders force-pushed the tui-open-log-in-external-tool branch from c51d584 to 38b4c09 Compare February 18, 2025 16:57
@oliver-sanders
Copy link
Member Author

For coverage, I think you could test this with a fake editor that just reads the file the in, then exits?

I've added an integration test that mocks out the command itself. This isn't really testing very much, but I think it's about all we can do without actually driving an interactive terminal session.

@oliver-sanders @hjoliver
Update changes.d/6611.feat.md
86b0b58 
Co-authored-by: Hilary James Oliver <[email protected]>
hjoliver

This comment was marked as resolved.

Sign in to view

@oliver-sanders

This comment was marked as resolved.

Sign in to view
@hjoliver

This comment was marked as resolved.

Sign in to view
@oliver-sanders

This comment was marked as resolved.

Sign in to view
@hjoliver

This comment was marked as resolved.

Sign in to view
@hjoliver

This comment was marked as resolved.

Sign in to view
@oliver-sanders

This comment was marked as resolved.

Sign in to view
hjoliver
hjoliver approved these changes Feb 27, 2025
View reviewed changes
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.

All good (noting tests need updating now).

@hjoliver

This comment was marked as resolved.

Sign in to view
@oliver-sanders
Copy link
Member Author

All good (noting tests need updating now).

Dammit, I did update the tests, but CI has $EDITOR configured differently (derp), needed to standardise the environment. Sorted.

@markgrahamdawson
Copy link
Contributor

I tested with Gedit (export GEDITOR=gedit in bash.rc) and VScode (export GEDITOR=code) in .bashrc
gedit worked fine but vs code just opens up a blank file 🤔

@oliver-sanders
Copy link
Member Author

Works for me, use GEDITOR='code --wait'.

  • A new tab will open in vscode.
  • Tui will background whilst the tab is open.
  • Tui will restore once the tab is closed.

@oliver-sanders oliver-sanders merged commit e42cf54 into cylc:master Mar 4, 2025
23 checks passed
@oliver-sanders oliver-sanders deleted the tui-open-log-in-external-tool branch March 4, 2025 17:26
wxtim added a commit to wxtim/cylc that referenced this pull request Mar 5, 2025
@wxtim
Merge remote-tracking branch 'upstream/master' into feat.skip_shown_i…
1d836ec 
…n_cylc_show

* upstream/master: (27 commits)
  tui: open log files in external application (cylc#6611)
  fix typo in stop.py - options.max_poll -> options.max_polls
  schema: add first-parent descendants (cylc#6610)
  doc: improve NamespaceIDGlob description (cylc#6637)
  Document that "ex" prefix means "exclude" [skip ci]
  tests: remove string templating in SQL statements (cylc#6631)
  Merge pull request cylc#6629 from wxtim/tests.flake8
  Remove vestiges of authorisation layer removed in cylc#3845
  Bump dev version
  Prepare release 8.4.1
  Merge pull request cylc#6578 from MetRonnie/graphql-err-handling
  Add test
  Update cylc/flow/etc/examples/expiry/index.rst
  Correct type annotations
  Fix duplicate task triggers
  Add test for duplicate task triggers
  Pytest: full verbosity for assertions
  Fix test picking up user global config
  Wrapper script: fix `PATH` override preventing selection of Cylc version in GUI under Cylc Hub
  Get poll to return task failure if job/log has been removed. (cylc#6577)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hjoliver hjoliver hjoliver approved these changes

@markgrahamdawson markgrahamdawson markgrahamdawson approved these changes

Assignees

@oliver-sanders oliver-sanders

Labels
small
Projects
None yet
Milestone
8.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@oliver-sanders @hjoliver @markgrahamdawson