Allow Template upgrade properties to be set and remove properties that no longer exist.#4783
Open
JC-wk wants to merge 50 commits into
Open
Allow Template upgrade properties to be set and remove properties that no longer exist.#4783JC-wk wants to merge 50 commits into
JC-wk wants to merge 50 commits into
Conversation
This commit aligns the resource upgrade process with the update process by correctly handling conditional properties in the JSON schema. - The schema generation logic in `ConfirmUpgradeResource.tsx` is updated to include conditional blocks (`if`/`then`/`else`) when the condition is based on an existing property. - New read-only properties are now submitted during the upgrade process.
This commit aligns the resource upgrade process with the update process by correctly handling conditional properties in the JSON schema. - The schema generation logic in `ConfirmUpgradeResource.tsx` is updated to include conditional blocks (`if`/`then`/`else`) when the condition is based on an existing property. - New read-only properties are now submitted during the upgrade process. - The `liveOmit` prop is added to the form to prevent the submission of unevaluated properties from conditionally hidden fields.
…1346005040390942732 Fix Upgrade Conditional Properties
Unit Test Results933 tests 933 ✅ 37s ⏱️ Results for commit dc1714f. ♻️ This comment has been updated with latest results. |
fix: allow upgrades on hidden properties
…property visibility
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enhances the Azure TRE upgrade flow so that template upgrades can accept values for newly introduced template properties and remove properties that no longer exist in the target template, reducing upgrade failures and lingering stale properties.
Changes:
- UI: adds an upgrade-time form to capture values for newly added template properties (including handling hidden properties/defaults).
- API: adjusts patch validation to allow setting newly introduced (previously non-existent) properties during an upgrade and removes properties absent from the new template.
- Tests/versioning: adds/updates unit tests and increments UI/API versions; updates the changelog.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/app/src/components/shared/ConfirmUpgradeResource.tsx | Fetches current/target template schemas to detect added/removed properties and renders an upgrade-time form for new properties. |
| ui/app/src/components/shared/ConfirmUpgradeResource.test.tsx | Extends UI tests to cover schema loading, new/removed property messaging, and upgrade payload contents. |
| api_app/db/repositories/resources.py | Allows new properties during upgrades, permits pipeline-substituted fields, and removes properties not present in the upgraded template. |
| api_app/tests_ma/test_db/test_repositories/test_resource_repository.py | Adds/updates repository validation tests for upgrade scenarios and pipeline property handling. |
| ui/app/package.json | Bumps UI version. |
| ui/app/package-lock.json | Updates lockfile version metadata to match UI version bump. |
| api_app/_version.py | Bumps API version. |
| CHANGELOG.md | Adds an enhancement entry describing the upgrade property behavior. |
Files not reviewed (1)
- ui/app/package-lock.json: Language not supported
Comment on lines
+37
to
+50
| // Utility to get all property keys from template schema's properties object recursively, flattening nested if needed | ||
| const getAllPropertyKeys = (properties: any, prefix = ""): string[] => { | ||
| if (!properties) return []; | ||
| let keys: string[] = []; | ||
| for (const [key, value] of Object.entries(properties)) { | ||
| if (value && typeof value === "object" && 'properties' in value) { | ||
| // recur for nested properties | ||
| keys = keys.concat(getAllPropertyKeys(value["properties"], prefix + key + ".")); | ||
| } else { | ||
| keys.push(prefix + key); | ||
| } | ||
| } | ||
| return keys; | ||
| }; |
Collaborator
Author
There was a problem hiding this comment.
If we had used the simplified Object.keys(properties) approach (Option 2) suggested by Copilot:
• An upgrade from a template with parent: { child } to parent: { child, sibling } would result in both
versions having the top-level key parent .
• Comparing them would yield [] (no new properties detected).
• The upgrade would proceed silently without giving the user a chance to supply a value for the new sibling
property, causing backend validation failures or incomplete deployments.
By keeping the recursive dotted-path detection but updating the form state management, validation, and API call
generation to handle nested objects correctly:
- Accurate Detection: We correctly identify when a nested property (e.g., parent.sibling ) is added/removed.
- Object Structure Alignment: We keep newPropertyValues in the nested object shape that the RJSF Form and
the backend PATCH API expect (e.g., { parent: { child: '...', sibling: '...' } } ). - Data Preservation: We pre-populate existing sibling values (e.g., keeping parent.child ) so they aren't lost
or cleared during the upgrade. - Validation Resolution: We use ConfirmUpgradeResource.tsx to check if the new nested fields are filled,
preventing the
"Upgrade" button from being permanently disabled.
Comment on lines
+292
to
+300
| // prefill newPropertyValues with schema defaults or empty string (excluding pipeline properties) | ||
| setNewPropertyValues( | ||
| newPropKeysToSend.reduce((acc, key) => { | ||
| // Get top-level portion of the key | ||
| const topKey = key.split('.')[0]; | ||
| const defaultValue = newTemplate?.properties?.[topKey]?.default; | ||
| acc[key] = defaultValue !== undefined ? defaultValue : ''; | ||
| return acc; | ||
| }, {} as any), |
Comment on lines
+454
to
+460
| primaryDisabled={ | ||
| !selectedVersion || | ||
| (newPropertiesToFill.length > 0 && | ||
| newPropertiesToFill.some( | ||
| (key) => newPropertyValues[key] === "" || newPropertyValues[key] === undefined, | ||
| )) | ||
| } |
Comment on lines
+114
to
+122
| def _get_all_property_keys_from_template(self, resource_template: ResourceTemplate) -> set: | ||
| properties = set(resource_template.properties.keys()) | ||
| if "allOf" in resource_template: | ||
| for condition in resource_template.allOf: | ||
| if "then" in condition and "properties" in condition["then"]: | ||
| properties.update(condition["then"]["properties"].keys()) | ||
| if "else" in condition and "properties" in condition["else"]: | ||
| properties.update(condition["else"]["properties"].keys()) | ||
| return properties |
|
|
||
| if resource_patch.templateVersion is not None: | ||
| await self.validate_template_version_patch(resource, resource_patch, resource_template_repo, resource_template, force_version_update) | ||
| new_template = await resource_template_repo.get_template_by_name_and_version(resource.templateName, resource_patch.templateVersion, resource.resourceType) |
| # get the schema for the target version if upgrade is happening | ||
| if resource_patch.templateVersion is not None: | ||
| # fetch the template for the target version | ||
| target_template = await resource_template_repo.get_template_by_name_and_version(resource_template.name, resource_patch.templateVersion, resource_template.resourceType) |
| * Remove Windows 10 and dsvm image support from Guacamole. ([#4890](https://github.com/microsoft/AzureTRE/issues/4890)) | ||
|
|
||
| ENHANCEMENTS: | ||
| * Allow new template properties to be specified during template upgrades. Remove Template properties that no longer exist. |
Comment on lines
+681
to
+685
| // Click upgrade | ||
| await waitFor(() => { | ||
| const upgradeButton = screen.getByTestId("primary-button"); | ||
| fireEvent.click(upgradeButton); | ||
| }); |
Enhance template upgrade functionality and improve resource handling - Allow new template properties to be specified during upgrades and remove obsolete properties. - Update resource repository methods to handle parent service names for user resources. - Add tests for nested properties and enum value handling in upgrade scenarios. - Refactor ConfirmUpgradeResource component to manage nested properties and validation more effectively. - Update TypeScript configuration for improved compatibility.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| return false; | ||
| }); | ||
| const removedPropsArray = currentKeys.filter((k) => !newKeys.includes(k)); |
Comment on lines
+338
to
+343
| const newSchemaProps = newTemplate?.properties || {}; | ||
| const currentProps = currentTemplate?.properties || {}; | ||
|
|
||
| const newKeys = getAllPropertyKeys(newSchemaProps); | ||
| const currentKeys = getAllPropertyKeys(currentProps); | ||
| const newPropKeys = newKeys.filter((key) => { |
…attr built-in function:
if hasattr(resource_template, "allOf") and resource_template.allOf is not None:
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #4732 #4730
What is being addressed
Currently if you add a new property to a template there is no way to specify this property before an upgrade is ran.
The upgrade may fail due to the missing property (although the template version is still incremented)
The user then has to click update and supply the property.
Similarly when removing a property from a template and running an upgrade, the property still exists on the resource.
Todo
How is this addressed