-
Notifications
You must be signed in to change notification settings - Fork 109
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
Enable differentiated error messages for DR eligibility #60
Conversation
@@ -53,6 +53,10 @@ async function run(): Promise<void> { | |||
} | |||
} catch (error) { | |||
if (error instanceof RequestError && error.status === 404) { | |||
core.setFailed( | |||
`Dependency review could not obtain dependency data for the specified owner, repository, or revision range.` |
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.
`Dependency review could not obtain dependency data for the specified owner, repository, or revision range.` | |
`Dependency review could not obtain dependency data for the specified owner, repository, or revision range. Please ensure that Dependency graph is enabled, see https://github.com/${github.context.repo.owner}/${github.context.repo.repo}/settings/security_analysis.` |
WDYT? I'd like to keep the instructions for enabling DG.
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.
I think this is the goal of the parent Issue, to differentiate between explicitly not-DR-eligible repos (403) and input problems to the API (bad repo/owner, bad commit/rev range, etc.) (404) which I don't expect to happen much when the inputs come from PRs. My estimate, you'll see those 404s drop off to nothing for DR Action callers, and the 403's will increase. Does that make sense?
OTOH - the messages themselves may or may not be what we want to relate to end users. Opinions about what 404 (I can't find the repo/owner or those revs don't exist) vs. 403 (this repo shouldn't have the DR Action installed unless you make it eligible for DR!)? cc @febuiles @courtneycl @lseppala
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.
@elireisman Apologies, I thought the error message for DG enablement had been lost but now I see it's been moved to the 403 case!
@@ -53,6 +53,10 @@ async function run(): Promise<void> { | |||
} | |||
} catch (error) { | |||
if (error instanceof RequestError && error.status === 404) { | |||
core.setFailed( | |||
`Dependency review could not obtain dependency data for the specified owner, repository, or revision range.` |
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.
@elireisman Apologies, I thought the error message for DG enablement had been lost but now I see it's been moved to the 403 case!
Done - and I just bumped the |
@elireisman |
Description
Addresses cases when DR eligibility checks fail on the API side, but the DR Action was not able to differentiate and supply specific instruction to end users.