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

Don't log/comment errors, require an admin to look at the logs. #114

Merged
merged 3 commits into from
Aug 8, 2022

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Aug 4, 2022

We recently leaked our environment-specified tokens in the log by dumping a Process object, so this PR includes a commit that removes all error reporting from the log generation. Instead, we'll now just comment that the job has failed, and expect the admin to check the server logs. cc @vtjnash

@vtjnash
Copy link
Member

vtjnash commented Aug 4, 2022

Seems to make sense to me. We don't include enough info here to be useful, but too much could be more likely to risk printing a secret

@maleadt
Copy link
Member Author

maleadt commented Aug 8, 2022

Exception objects also leaked in another code path, e.g. JuliaLang/julia#46075 (comment), so I removed that and streamlined the error reporting. The run methods can now just throw, and only the server will catch those exceptions and turn them into a generic something failed error. This also makes it more reliable to execute the run methods during testing, where previously they wouldn't actually error but only generate a log with errors in them.

@maleadt maleadt changed the title PkgEval-related fixes Don't log/comment errors, require an admin to look at the logs. Aug 8, 2022
@maleadt maleadt merged commit 20c0c4c into master Aug 8, 2022
@maleadt maleadt deleted the tb/pkgeval_fixes branch August 8, 2022 10:10
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.

2 participants