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

issue-1606-Add dev-null redirect to suppress error #1608

Closed
wants to merge 1 commit into from

Conversation

numberwhun
Copy link

@numberwhun numberwhun commented Oct 9, 2023

…rs on some systems when running lsb_release

This PR is a proposal to fix #1606 by adding a redirect to dev-null to suppress output of an unneeded and unwanted message from lsb_release. By redirecting the output to dev-null, you will again get the desired result, fixing this issue in current versions, such as Debian, where the message is output regardless.

Please review and merge.

Checklist

  • [ x ] based on top of latest source code Repo was freshly forked and checked out.
  • [ x ] changelog entry included Not sure if the change is all that exciting, its just a simple redirect that is needed. I can add if needed
  • automated tests pass
  • git history is clean
  • [ x ] git commit messages are well-written

@numberwhun numberwhun changed the title issue-1606 - Adding redirect to dev-null for error message that appea… issue-1606 - Adding redirect to dev-null to suppress error message Oct 9, 2023
@numberwhun numberwhun changed the title issue-1606 - Adding redirect to dev-null to suppress error message issue-1606-Add dev-null redirect to suppress error Oct 9, 2023
@numberwhun numberwhun marked this pull request as ready for review October 9, 2023 04:26
@dboehmer
Copy link

I confirmed that this PR fixes the issue #1606 in my case! 👍

I think the commit message itself should contain all the info given in this PR’s description, at least the issue ID. The current commit message is not helpful to understand why it was changed.

@ferki
Copy link
Member

ferki commented Oct 12, 2023

Thank you, @numberwhun for this PR, and @dboehmer for initial tests!

As I commented on the original issue, I don't think this is a bug, but a missing feature flag to be enabled if distinction between STDERR and STDOUT streams are desired.

I normally don't jump to the implementation phase until I fully understand a situation. Based on the investigation of the original issue, I don't yet feel a change is warranted, so I did not do a full review yet.

Still, it's small change, so here's some quick notes as first feedback from most important to least:

  • new tests are missing for the supposed problematic behavior, so we can't demonstrate a failing case, nor verify if it's correctly solved, or whether it's accidentally reintroduced in the future
  • the i_run command has a built-in no_stderr option instead of manually appending 2> /dev/null (which may or may not be OS-specific detail)
  • a user-visible change is proposed, so a changelog entry would be required
  • the git diff tells us what changes and how, but only the git commit can tell us why; the current one does not add more info on top of the diff, so it probably can be improved (e.g. "Fix OS detection without LSB modules" or similar)

In any case, let's see if we need to change something in the first place, and if yes, whether that is suppressing STDERR as a whole or something else.

@ferki
Copy link
Member

ferki commented Oct 20, 2023

The original issue targeted here has been closed without requiring modification to the code, so I'm closing this PR too.

Nevertheless, thanks again for your contribution!

I hope to continue our discussions on one of our support channels, and look forward to further collaboration.

@ferki ferki closed this Oct 20, 2023
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.

OS detection fails for Debian Bookworm
3 participants