| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
Updates repository property handling to support API responses where value can be a string or string[], and validates/parses only properties that this action recognizes to avoid failing on unrelated properties.
Changes:
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/feature-flags/properties.ts | Introduces RepositoryPropertyValue and PropertyInfo-based validation/parsing to avoid throwing on unknown properties and to support future typed properties. |
| src/feature-flags/properties.test.ts | Updates existing tests and adds coverage ensuring unknown properties with unexpected value types don’t throw. |
| lib/init-action.js | Regenerated compiled bundle reflecting the TS changes. |
Sorry, something went wrong.
There was a problem hiding this comment.
Looks good! Consider adding a changelog note.
Sorry, something went wrong.
| @@ -86,14 +126,6 @@ export async function loadPropertiesFromApi( | |||
| } | |||
|
|
|||
| if (isKnownPropertyName(property.property_name)) { | |||
| // Only validate the type of `value` if this is a property we care about, to avoid throwing | |||
| // on unrelated properties that may use representations we do not support. | |||
| if (typeof property.value !== "string") { | |||
| throw new Error( | |||
| `Expected repository property '${property.property_name}' to have a string value, but got: ${JSON.stringify(property)}`, | |||
| ); | |||
| } | |||
|
|
|||
| setProperty(properties, property.property_name, property.value, logger); | |||
| } | |||
| } | |||
| @@ -119,14 +151,30 @@ export async function loadPropertiesFromApi( | |||
| } | |||
| } | |||
|
|
|||
| /** Update the partial set of repository properties with the parsed value of the specified property. */ | |||
| /** | |||
| * Validate that `value` has the correct type for `K` and, if so, update the partial set of repository | |||
| * properties with the parsed value of the specified property. | |||
| */ | |||
| function setProperty<K extends RepositoryPropertyName>( | |||
| properties: RepositoryProperties, | |||
| name: K, | |||
| value: string, | |||
| value: RepositoryPropertyValue, | |||
| logger: Logger, | |||
| ): void { | |||
| properties[name] = repositoryPropertyParsers[name](name, value, logger); | |||
| const propertyOptions = repositoryPropertyParsers[name]; | |||
|
|
|||
| // We perform the validation here for two reasons: | |||
| // 1. This function is only called if `name` is a property we care about, to avoid throwing | |||
| // on unrelated properties that may use representations we do not support. | |||
| // 2. The `propertyOptions.validate` function checks that the type of `value` we received from | |||
| // the API is what expect and narrows the type accordingly, allowing us to call `parse`. | |||
| if (propertyOptions.validate(value)) { | |||
| properties[name] = propertyOptions.parse(name, value, logger); | |||
| } else { | |||
| throw new Error( | |||
| `Unexpected value for repository property '${name}', got: ${JSON.stringify(value)}`, | |||
| ); | |||
| } | |||
| } | |||
|
|
|||
| /** Parse a boolean repository property. */ | |||
| Back | FazBrowse Home | New Git URL |
Fixes #3555.
The repository property API will return repository property objects where the value field is either string or string[]. We do not currently handle the latter.
This PR makes the following changes:
These two commits are enough to fix the bug. The third commit goes beyond this to enable us to use array-typed properties in the future by:
Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
Products:
Environments:
How did/will you validate this change?
If something goes wrong after this change is released, what are the mitigation and rollback strategies?
The repository properties feature is already FF-ed. If something goes wrong with this change, we can either disable the existing FF or perform a rollback.
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