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.

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

audit: report any errors above 400 as potentially not supporting audit #128

Merged
merged 1 commit into from
Jan 9, 2019
Merged

audit: report any errors above 400 as potentially not supporting audit #128

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions lib/audit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,16 @@ function auditCmd (args, cb) {
}).then((auditReport) => {
return audit.submitForFullReport(auditReport)
}).catch((err) => {
if (err.statusCode === 404 || err.statusCode >= 500) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

401 needs to not do this, as the main registry is (or is about to) start returning 401s when your auth data is bad, regardless of endpoint.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest that at the very least, we catch 404, 401 and then all other errors separately and tune the error messages to align better, eg:

401 - "Either your login credentials are invalid or your registry does not support audit."
404 - "Your registry does not support audit."
all other - what you have now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed this to follow your suggestion. You can re-review at your convenience.

const ne = new Error(`Your configured registry (${opts.registry}) does not support audit requests.`)
if (err.statusCode >= 400) {
let msg
if (err.statusCode === 401) {
msg = `Either your login credentials are invalid or your registry (${opts.registry}) does not support audit.`
} else if (err.statusCode === 404) {
msg = `Your configured registry (${opts.registry}) does not support audit requests.`
} else {
msg = `Your configured registry (${opts.registry}) does not support audit requests, or the audit endpoint is temporarily unavailable.`
}
const ne = new Error(msg)
ne.code = 'ENOAUDIT'
ne.wrapped = err
throw ne
Expand Down