Skip to content

fix: export command info log moved to only log when outputting a file #150

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
Merged
ashearin merged 2 commits into from
Sep 5, 2024
Merged

fix: export command info log moved to only log when outputting a file #150

ashearin merged 2 commits into from
Sep 5, 2024

Conversation

@idunbarh
Copy link
Member

@idunbarh idunbarh commented Sep 5, 2024 •
edited
Loading

Description

This PR fixes #149.

When piping bomctl export ... to another command, we should not be printing logging info to stdout. This is cause SBOM schema failures on the tools receiving the sbom content.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

./bomctl fetch https://raw.githubusercontent.com/bomctl/bomctl-playground/main/examples/bomctl-container-image/bomctl_bomctl_v0.3.0.cdx.json
./bomctl ls
./bomctl export urn:uuid:f360ad8b-dc41-4256-afed-337a04dff5db
./bomctl export urn:uuid:f360ad8b-dc41-4256-afed-337a04dff5db | grype

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

@idunbarh
fix: export command info log moved to only log when outputting a file
faa2869 
Signed-off-by: Ian Dunbar-Hall <ian.dunbar-hall@lmco.com>
@idunbarh idunbarh requested a review from a team as a code owner September 5, 2024 04:41
@jhoward-lm
Copy link
Contributor

jhoward-lm commented Sep 5, 2024 •
edited
Loading

I could see this issue potentially having a more widespread impact. I don't have any problems with this solution, but @idunbarh @ashearin how would you feel about sending all console logging to stderr to avoid this popping up again?

Basically in logger.go, change this to use os.Stderr instead

logger := log.NewWithOptions(os.Stdout, log.Options{Prefix: prefix, Level: log.GetLevel()})

I tested by making this change and running

bomctl fetch <URL> --verbose --netrc               # prints output
bomctl fetch <URL> --verbose --netrc 2> /dev/null  # suppresses output
bomctl fetch <URL> --verbose --netrc > test.txt    # prints output to console and writes nothing to test.txt

@idunbarh
fix: changed logs to be sent to stderr
c1f11f4 
Signed-off-by: Ian Dunbar-Hall <ian.dunbar-hall@lmco.com>
@idunbarh
Copy link
Member Author

idunbarh commented Sep 5, 2024

I could see this issue potentially having a more widespread impact. I don't have any problems with this solution, but @idunbarh @ashearin how would you feel about sending all console logging to stderr to avoid this popping up again?

Basically in logger.go, change this to use os.Stderr instead

logger := log.NewWithOptions(os.Stdout, log.Options{Prefix: prefix, Level: log.GetLevel()})

I tested by making this change and running

bomctl fetch <URL> --verbose --netrc               # prints output
bomctl fetch <URL> --verbose --netrc 2> /dev/null  # suppresses output
bomctl fetch <URL> --verbose --netrc > test.txt    # prints output to console and writes nothing to test.txt

Great suggestion! Updated to change logs to stderr.

@ashearin ashearin merged commit 725501e into main Sep 5, 2024
@ashearin ashearin deleted the export-stdout branch September 5, 2024 16:49
pkwiatkowski1 pushed a commit to pkwiatkowski1/bomctl that referenced this pull request Sep 5, 2024
@idunbarh @pkwiatkowski1
fix: export command info log moved to only log when outputting a file (…
5361b3f 
…bomctl#150)

* fix: changed logs to be sent to stderr

Signed-off-by: Ian Dunbar-Hall <ian.dunbar-hall@lmco.com>

---------

Signed-off-by: Ian Dunbar-Hall <ian.dunbar-hall@lmco.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ashearin ashearin ashearin approved these changes

@jhoward-lm jhoward-lm jhoward-lm approved these changes

@houdini91 houdini91 Awaiting requested review from houdini91

@eddiezane eddiezane Awaiting requested review from eddiezane

@lmphil lmphil Awaiting requested review from lmphil

@mfrystacky mfrystacky Awaiting requested review from mfrystacky

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Export command piping of stdout causes issues from logging

4 participants

@idunbarh @jhoward-lm @ashearin