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

Enable differentiated error messages for DR eligibility #60

Merged
merged 2 commits into from May 12, 2022

Conversation

elireisman
Copy link
Contributor

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.

@elireisman elireisman requested a review from a team as a code owner May 11, 2022 19:56
@elireisman elireisman self-assigned this May 11, 2022
@@ -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

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!

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

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!

@elireisman elireisman merged commit 39e692f into main May 12, 2022
@elireisman
Copy link
Contributor Author

Done - and I just bumped the v1 tag to latest. FYI - this might pick up more commits than just my own ❗ it looks like it was still pinned to the original init commit for this repo. I'm watching the API graphs today 🤞 should be fine?

@febuiles
Copy link
Contributor

@elireisman v1 was used for launch, but going forward we plan to use semver for this Action. Next release should be 1.0.1. Changes on v1 should still be picked up I think.

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.

None yet

2 participants