-
Notifications
You must be signed in to change notification settings - Fork 73
CVE Feed: Handle recoverable errors #154
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
Conversation
|
/hold until #151 merges |
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.
/lgtm
only a few nits but I'm not sure those are even relevant.
| python3 fetch-official-cve-feed.py | tee $OUTPUT_FILE | ||
| python3 fetch-official-cve-feed.py > "${OUTPUT_FILE}" | ||
| EXIT_CODE=$? | ||
| if [ $EXIT_CODE -ne 0 ]; 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.
we've been using double brackets in the rest of the script, should we use it everywhere, do we care?
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.
This is a very good question!
The existence of [ and [[ in bash is a historical accident (the former comes from historical bourne shell, the latter from historical korn shell) and they "should" be identical unless you're doing something inappropriately clever. :-D
I'll go ahead and make everything [[ for the sake of uniformity.
|
|
||
| # python returns 7 to indicate recoverable errors | ||
| # Exit bash script now if unrecoverable python error | ||
| if [ $EXIT_CODE -ne 0 ] && [ $EXIT_CODE -ne 7 ]; 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.
again not expert in bash, but should we double quote all those vars?
| python3 fetch-official-cve-feed.py > "${OUTPUT_FILE}" | ||
| EXIT_CODE=$? | ||
| if [ $EXIT_CODE -ne 0 ]; then | ||
| RETURN_VALUE=$EXIT_CODE |
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.
or even this "$EXIT_CODE"?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mtardy, tabbysable 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 |
|
I'll let you unhold this when you think it's ready :) I think it looks fine but you are the bash expert here |
|
Honestly, it would be good practice to run it through shellcheck and do all the things the linter suggests. I'll plan to do that >< |
|
/lgtm |
|
/unhold |
For logging of errors, propagate python script return code through shell to Prow.
This should allow an email to be sent to job maintainers even in the case where the python script continued through a recoverable error.
(Per discussions during SIG Security Tooling working session)