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

ssh: don't use a separate logger #250

Merged
merged 3 commits into from
Sep 4, 2024
Merged

ssh: don't use a separate logger #250

merged 3 commits into from
Sep 4, 2024

Conversation

ydirson
Copy link
Contributor

@ydirson ydirson commented Aug 29, 2024

The logger setup was confusing when using --log-file-level=DEBUG, showing this debug-level output inside default --log-cli-level=INFO output, but without the debug context that only went into the logfile, and without any hint of what kind of output it was.

This makes it use the same logger as all other debug output, preventing it from appearing in the wrong place, and adds a marker to hint it is output data - at the same time fixing a potential formatting issue, where arbitrary ssh output lines were interpreted as format strings.

@benjamreis
Copy link
Contributor

If the output look ok to everyone - fine by me :)

@stormi
Copy link
Member

stormi commented Sep 2, 2024

I'm OK on the principle, but I want to run some tests.

The logger setup was confusing when using --log-file-level=DEBUG,
showing this debug-level output inside default --log-cli-level=INFO output,
but without the debug context that only went into the logfile, and without
any hint of what kind of output it was.

This makes it use the same logger as all other debug output, preventing it
from appearing in the wrong place, and adds a marker to hint it is output
data - at the same time fixing a potential formatting issue, where
arbitrary ssh output lines were interpreted as *format strings*.

Signed-off-by: Yann Dirson <yann.dirson@vates.tech>
…logs"

This reverts commit 5b97ff0.

With the unified logger the original issue is not there any more.

Signed-off-by: Yann Dirson <yann.dirson@vates.tech>
They won't make it to the console unless required by --log-cli-level, but
now pytest will show them systematically when a test fails.

Signed-off-by: Yann Dirson <yann.dirson@vates.tech>
Copy link
Member

@stormi stormi left a comment

Choose a reason for hiding this comment

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

LGTM. I had a weird output where I'd get the failed quicktest output after the captured test and teardown output, but couldn't reproduce, with or without -s.

@stormi stormi merged commit 56fa202 into master Sep 4, 2024
4 checks passed
@stormi stormi deleted the ssh-logging branch September 4, 2024 16:07
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