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

Redirect everything that was spamming TTY1 to Journal #5842

Merged
merged 3 commits into from
Aug 21, 2024
Merged

Redirect everything that was spamming TTY1 to Journal #5842

merged 3 commits into from
Aug 21, 2024

Conversation

M4rtinK
Copy link
Contributor

@M4rtinK M4rtinK commented Aug 20, 2024 •
edited
Loading

So far unsuccessfully. :P

Works now. :)

@M4rtinK M4rtinK added blocked Don't merge this pull request! rhel-10 labels Aug 20, 2024
@M4rtinK
Copy link
Contributor Author

M4rtinK commented Aug 20, 2024

redirect_mk1
redrect_mk1_tty1

@M4rtinK M4rtinK force-pushed the rhel-10-redirect_EVERYTHING branch from 55d5b9f to 3f90450 Compare August 21, 2024 11:14
@pep8speaks
Copy link

pep8speaks commented Aug 21, 2024 •
edited
Loading

Hello @M4rtinK! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-08-21 12:20:56 UTC

@M4rtinK M4rtinK changed the title Try to redirect everything that could be spamming TTY1 Redirect everything that was spamming TTY1 to Journal Aug 21, 2024
@M4rtinK M4rtinK added port to Fedora and removed blocked Don't merge this pull request! labels Aug 21, 2024
@M4rtinK
Copy link
Contributor Author

M4rtinK commented Aug 21, 2024

Should be working now:
solved_tui
This is how RDP client connecting and disconnecting looks like in Journal:
solved

@M4rtinK M4rtinK marked this pull request as ready for review August 21, 2024 11:17
@M4rtinK M4rtinK force-pushed the rhel-10-redirect_EVERYTHING branch from 3f90450 to 25d9edb Compare August 21, 2024 11:20
@M4rtinK
Copy link
Contributor Author

M4rtinK commented Aug 21, 2024

/kickstart-test --skip-testtypes whatever

@M4rtinK
Copy link
Contributor Author

M4rtinK commented Aug 21, 2024

Thanks a lot to @halfline & @jstodola for helping to track down the root cause of this! :)

@M4rtinK
Redirect Anaconda main process stderr to Journal
9406a6a 
This should prevent error messages (at the moment mostly
from GTK, which stil runs in the main thread) from swamping
the TUI running on TTY1.

This also has the added benefit of any such errors now being captured
in the Journal, which is usually stored from any test runs
and easy for customers to send back if they encounter an issue.

And thanks a lot to Ray Strode for helping us track this down! :)

Resolves: RHEL-47097
@M4rtinK
Remove leftover debugging message
fcb5154 
The address is printed correctly a few line below.

Resolves: RHEL-47097
@M4rtinK
Redirect output of various GNOME related tools to Journal
487a26b 
This should avoid the output spamming TUI on TTY1 & any errors will
be captured in Journal dumps.

Resolves: RHEL-47097
@M4rtinK M4rtinK force-pushed the rhel-10-redirect_EVERYTHING branch from 25d9edb to 487a26b Compare August 21, 2024 12:20
@M4rtinK
Copy link
Contributor Author

M4rtinK commented Aug 21, 2024

Turns out the accelerators fix was hitting an issue with some dodgy GTK API (Gtk.AccelGroup.query() - have not been able to find authoritative docs for Python & GTK for this), so I've dropped that commit for now. Given all the redirections to Journal, this commit is not totally necessary to keep TTY1 clean. :)

@M4rtinK
Copy link
Contributor Author

M4rtinK commented Aug 21, 2024

/kickstart-test --testtype smoke

@M4rtinK
Copy link
Contributor Author

M4rtinK commented Aug 21, 2024

/kickstart-test --skip-testtypes whatever

@M4rtinK M4rtinK mentioned this pull request Aug 21, 2024
Closed
jkonecny12
jkonecny12 approved these changes Aug 21, 2024
View reviewed changes
Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

Looks good to me. However, it's problematic that we don't have test coverage for anything here.

rvykydal
rvykydal approved these changes Aug 21, 2024
View reviewed changes
Copy link
Contributor

@rvykydal rvykydal left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@M4rtinK
Copy link
Contributor Author

M4rtinK commented Aug 21, 2024 •
edited
Loading

Looks good to me. However, it's problematic that we don't have test coverage for anything here.

Yeah - not sure about the run-in-new-session script itself for example, given the amount of black magic involved - hopefully we can make the installation environment more sane over time & make it unnecessary. :)

But for display.py or gnome_remote_desktop.py - that definitely could and should be covered by unit tests. Definitely something to keep in mind going forward. :)

@M4rtinK M4rtinK merged commit d196ffb into rhinstaller:rhel-10 Aug 21, 2024
10 of 11 checks passed
@jkonecny12 jkonecny12 mentioned this pull request Aug 21, 2024
Merged
9 tasks
@jkonecny12
Copy link
Member

It's now forwardported to Fedora in #5829

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkonecny12 jkonecny12 jkonecny12 approved these changes

@rvykydal rvykydal rvykydal approved these changes

Assignees
No one assigned
Labels
rhel-10
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@M4rtinK @pep8speaks @jkonecny12 @rvykydal