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

Enable differentiated error messages for DR eligibility #60

Merged
merged 2 commits into from May 12, 2022
Merged

Enable differentiated error messages for DR eligibility #60

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cabd238
enable differentiated error messages for DR eligibility when API retu…
elireisman May 11, 2022
7dcdeab
update dist
elireisman May 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions dist/index.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/index.js.map
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 4 additions & 0 deletions src/main.ts
View file
Open in desktop
Expand Up @@ -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.`
Copy link
Contributor

@febuiles febuiles May 12, 2022 •
edited

Choose a reason for hiding this comment

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

Suggested change
`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.

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 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

Copy link
Contributor

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!

)
} else if (error instanceof RequestError && error.status === 403) {
core.setFailed(
`Dependency review is not supported on this repository. Please ensure that Dependency graph is enabled, see https://github.com/${github.context.repo.owner}/${github.context.repo.repo}/settings/security_analysis`
)
Expand Down