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

cat-log: view retrieved remote logs locally. #2624

Merged
merged 3 commits into from
Apr 27, 2018

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Apr 17, 2018

A small follow-up to #2503, to avoid unnecessary ssh connections. Current cat-log tests all pass on this branch with log retrieval or not configured in global-tests.rc, but before requesting review I'll add another test to distinguish the two cases.

@hjoliver hjoliver added this to the next release milestone Apr 17, 2018
@hjoliver hjoliver self-assigned this Apr 17, 2018
@hjoliver
Copy link
Member Author

tests added

Copy link
Contributor

@matthewrmshin matthewrmshin left a comment

Choose a reason for hiding this comment

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

Test good in my environment. Some minor comments.

bin/cylc-cat-log Outdated
Print, edit, or tail-follow content, or print path or list directory, of local
or remote task job logs and suite server logs. Batch-system-specific job cat or
tail commands are used if defined in global config.
Cat (print), edit, or tail-follow content, or print path or list directory, of
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Print is actually more correct here.

The original Unix cat command does concatenate files and print on the standard output. Correct me if I am wrong, but I don't think this command actually does concatenate.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think unix cat is almost universally used to print a single file in practice - but fair point, you're right about it's origins and functionality. I'll correct the command help ... but this does suggest a bigger problem in that our command name is cylc cat-log!

# Cat the remote one.
TEST_NAME=${TEST_NAME_BASE}-task-out
run_ok $TEST_NAME cylc cat-log -f o $SUITE_NAME a-task.1
grep_ok '^the quick brown fox$' ${TEST_NAME}.stdout
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to run purge_suite to tidy up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

# Cat the local one.
TEST_NAME=${TEST_NAME_BASE}-out-loc
run_ok $TEST_NAME cylc cat-log -f o $SUITE_NAME a-task.1
grep_ok '^the quick brown FOX$' ${TEST_NAME}.stdout
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to run purge_suite to tidy up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@matthewrmshin matthewrmshin 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.

Copy link
Collaborator

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

Sensible & logical change with suitable tests which pass locally.

@sadielbartholomew sadielbartholomew merged commit 999ab81 into cylc:master Apr 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants