Turn enablement errors into configuration errors#3251
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves error handling for GitHub Security feature enablement errors by providing more user-friendly messages when features like Code Security, Advanced Security, or Code Scanning are not enabled.
Key changes:
- Added an
isEnablementErrorhelper function to detect enablement-related error messages - Enhanced
wrapApiConfigurationErrorto wrap enablement errors with clearer guidance - Added comprehensive test coverage for the three enablement error scenarios
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/api-client.ts | Added isEnablementError helper and integrated enablement error checking into wrapApiConfigurationError |
| src/api-client.test.ts | Added test cases for Code Security, Advanced Security, and Code Scanning enablement errors |
| lib/upload-sarif-action.js | Generated JavaScript code from TypeScript source |
| lib/upload-lib.js | Generated JavaScript code from TypeScript source |
| lib/setup-codeql-action.js | Generated JavaScript code from TypeScript source |
| lib/init-action.js | Generated JavaScript code from TypeScript source |
| lib/init-action-post.js | Generated JavaScript code from TypeScript source |
| lib/analyze-action.js | Generated JavaScript code from TypeScript source |
esbena
left a comment
There was a problem hiding this comment.
LGTM with two minor things to consider.
| new util.ConfigurationError("Resource not accessible by integration"), | ||
| ); | ||
|
|
||
| // Enablement errors. |
There was a problem hiding this comment.
I find the test messages a bit brittle since they do exact matching on the user-facing texts, but I find it unlikely to result in significant churn, so 👍🏻
There was a problem hiding this comment.
I have wrapped this up in a function to avoid the duplication in case we want to change it down the line. Ideally, I would have put it in error-messages.ts but that currently (transitively) depends on api-client.ts and I didn't want to refactor that as part of this PR so I have left a TODO comment for it.
This PR adds more cases to
wrapApiConfigurationErrorwhere we want specific errors to be treated asConfigurationErrors.Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
analysis-kinds: code-scanning).upload-sarif).How did/will you validate this change?
.test.tsfiles).pr-checks).If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Merge / deployment checklist