Tolerate errors loading repository properties #3421
Conversation
There was a problem hiding this comment.
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
mbg
left a comment
There was a problem hiding this comment.
This is a great first stab at this, thank you! ✨
A few suggestions and questions, but generally this looks good.
| Explicitly enable or disable caching of project build dependencies. | ||
| required: false | ||
| repository-owner-type: | ||
| default: ${{ github.event.repository.owner.type }} |
There was a problem hiding this comment.
This seems like a reasonable solution that avoids an (additional) API call.
To check: whether repository is available for all event types we care about (especially dynamic).
Alternative ideas:
- Query the API, but we'd want to avoid that if it incurs an extra request. For this option, ideally we'd get this out of a call we have to make anyway.
- Check the status code of the response. If we get a 404, then it's probably because it is a user-owned repo.
| ); | ||
| } | ||
|
|
||
| async function loadRepositoryProperties( |
There was a problem hiding this comment.
Move this to init.ts or properties.ts, add a JSDoc, and some unit tests?
| // Fetch the values of known repository properties that affect us. | ||
| const enableRepoProps = await features.getValue( | ||
| Feature.UseRepositoryProperties, | ||
| const repositoryProperties = await loadRepositoryProperties( |
There was a problem hiding this comment.
Minor: Name this to indicate that it isn't a repository properties object itself, but a Result that may contain one? E.g. repositoryPropertiesResult.
| addDiagnostic( | ||
| config, | ||
| // Arbitrarily choose the first language. We could also choose all languages, but that | ||
| // increases the risk of misinterpreting the data. | ||
| config.languages[0], |
There was a problem hiding this comment.
Minor: We keep using this pattern; time to add a function for it so we don't have to repeat it / the comment every time?
| } | ||
|
|
||
| orElse(defaultValue: T): T { | ||
| return this._ok ? (this.value as T) : defaultValue; |
There was a problem hiding this comment.
Would TS accept isOk() instead of this._ok and narrow this.value to T so we don't need the as T?
| type Success<T> = Result<T, never>; | ||
| type Failure<E> = Result<never, E>; | ||
|
|
||
| export class Result<T, E> { |
There was a problem hiding this comment.
I like this -- it's a good idea to start including errors in the types.
Would you mind adding docs and unit tests for this?
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