-
Notifications
You must be signed in to change notification settings - Fork 433
Tolerate errors loading repository properties #3421
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
Tolerate errors loading repository properties #3421
Conversation
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.
Pull request overview
This PR introduces error tolerance for loading repository properties during the initial rollout phase. The change adds a Result type for better error handling, skips property loading for user-owned repositories (which cannot have custom properties), and logs diagnostic telemetry when property loading fails instead of causing the workflow to fail.
Changes:
- Introduced a generic
Result<T, E>type for error handling - Added
loadRepositoryPropertieshelper function that tolerates errors during property loading - Skips repository property loading for user-owned repositories
- Improved logging when no repository properties are found
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/util.ts | Adds a new Result class for functional error handling with ok/error states |
| src/init-action.ts | Refactors property loading into a separate function with error tolerance and adds diagnostic telemetry for failures |
| src/feature-flags/properties.ts | Improves logging to distinguish between no properties found vs properties loaded |
| init/action.yml | Adds repository-owner-type input to detect user-owned repositories |
| lib/init-action.js | Generated JavaScript output (mirrors TypeScript changes) |
mbg
left a comment
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.
This is a great first stab at this, thank you! ✨
A few suggestions and questions, but generally this looks good.
| ); | ||
| } | ||
|
|
||
| async function loadRepositoryProperties( |
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.
Move this to init.ts or properties.ts, add a JSDoc, and some unit tests?
mbg
left a comment
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.
Thank you for addressing all of my feedback! 🏆
This looks good to me. I added a couple of minor comments. Not blocking, but one to think about with respect to dynamic workflows if you haven't checked yet.
src/util.ts
Outdated
| ) {} | ||
|
|
||
| static ok<T>(value: T): Success<T> { | ||
| /** A success result. */ |
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.
Minor: These are the same comments as for the types, but these are actually functions constructing values of those types. How about "Constructs a success result", and similarly for the failure case below?
Repository properties can be used to configure the analysis. Therefore in general we probably don't want to tolerate errors loading repository properties, because this could mean that we run a different set of queries to what was requested, for instance.
However in the first stages of the rollout, tolerate errors.
Also, don't attempt to load repository properties for repositories owned by users, as these cannot have custom properties.
Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
dynamicworkflows (Default Setup, CCR, ...).Products:
analysis-kinds: code-scanning.analysis-kinds: code-quality.Environments:
github.comand/or GitHub Enterprise Cloud with Data Residency.How did/will you validate this change?
I've tested it on this repository, and will run another test on a repo owned by a user.
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?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist