-
Notifications
You must be signed in to change notification settings - Fork 73
Fix: Suppress Snyk debug logs and redirect to temp file #148
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
Fix: Suppress Snyk debug logs and redirect to temp file #148
Conversation
|
Welcome @4rivappa! |
mtardy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!! Looks good overall just a few questions
| RESULT_UNFILTERED=$(snyk test -d --json) || EXIT_CODE=$? | ||
| DEBUG_LOG_FILE=$(mktemp) | ||
| RESULT_UNFILTERED=$(snyk test -d --json 2> "$DEBUG_LOG_FILE") || EXIT_CODE=$? | ||
| if [ $EXIT_CODE -gt 1 ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kinda out of scope of this patch and not super familiar with this script or the error output code of snyk, but do you know if it's normal that we ignore the value 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it is normal to ignore the exit code 1 (case of vulnerabilities found) from snyk commands.
We are handling it in the below json parsing !
Description of exit codes from snyk docs
Possible exit codes and their meaning:
0: success (scan completed), no vulnerabilities found
1: action_needed (scan completed), vulnerabilities found
2: failure, try to re-run command. Use -d to output the debug logs.
3: failure, no supported projects detected
And to confirm that snyk debug logs are redirected to stderr, found the pointer to debugger init.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah thanks for the detailed answer, indeed 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we could early exit then on 0 but that's not super important
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For processing the containers scan, we need to proceed (cannot early exit, here) 😅
Previously, snyk debug output was printed directly to the console, cluttering CI logs. This change redirects stderr to a temporary log file, and only displays it if the snyk command fails. Signed-off-by: arivappa <4rivappa@proton.me>
f48b139 to
d56fdbb
Compare
| RESULT_UNFILTERED=$(snyk test -d --json) || EXIT_CODE=$? | ||
| DEBUG_LOG_FILE=$(mktemp) | ||
| RESULT_UNFILTERED=$(snyk test -d --json 2> "$DEBUG_LOG_FILE") || EXIT_CODE=$? | ||
| if [ $EXIT_CODE -gt 1 ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah thanks for the detailed answer, indeed 😅
| RESULT_UNFILTERED=$(snyk test -d --json) || EXIT_CODE=$? | ||
| DEBUG_LOG_FILE=$(mktemp) | ||
| RESULT_UNFILTERED=$(snyk test -d --json 2> "$DEBUG_LOG_FILE") || EXIT_CODE=$? | ||
| if [ $EXIT_CODE -gt 1 ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we could early exit then on 0 but that's not super important
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 4rivappa, mtardy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Let's ship this, I don't have a Snyk token or access to it to try this locally. We'll revert/fix if it's broken. /lgtm |
|
Here's the place to look https://prow.k8s.io/job-history/gs/kubernetes-ci-logs/logs/ci-kubernetes-snyk-master |
|
logs look lean! https://storage.googleapis.com/kubernetes-ci-logs/logs/ci-kubernetes-snyk-master/1950508691786567680/build-log.txt that's nice thanks! |
Resolves #146
Previously, snyk debug output was printed directly to the console, cluttering CI logs.
This change redirects stderr to a temporary log file, and only displays it if the
snyk commandfails.Applied changes to two snyk commands:
snyk test -d --jsonsnyk container test $image -d --jsonTesting and Validation:
I have tested changes applied to the first snyk command
snyk test -d --json,TODO: Need to test the entire script once, with k/k master.