Resource ready conditions #100
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds an optional readyCondition to ResourceDefinition, exposes resourceDefinition and entity in node contexts, migrates several type/service imports to @platform-mesh/portal-ui-lib, implements ready-condition-aware readiness computation and GraphQL field generation in the list view, and expands related tests and docs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…into resource-ready-conditions
| "namespace": null, | ||
| "readyCondition": { | ||
| "jsonPathExpression": "status.conditions[?(@.type=='Ready')].status", | ||
| "property": ["status.conditions.status", "status.conditions.type"], |
There was a problem hiding this comment.
i don't understand this configuration.
the jsonPathExpression ends up in a concrete value - a particular conditions stats.
What does the property then do?
There was a problem hiding this comment.
It's needed for graphQL. So they can be fetch from it
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
projects/lib/utils/utils/get-value-by-path.ts (1)
4-12: Consider constraining the generic type for better type safety.The function accepts any object type
Tbut immediately casts it toResourcewithout validation. Consider either removing the generic parameter or constrainingTto extendResourceto make the type contract more explicit.Example refactor:
-export const getValueByPath = <T extends object, R = unknown>( - obj: T, +export const getValueByPath = <R = unknown>( + obj: Resource, path: string, ): R | undefined => { - return getResourceValueByJsonPath(obj as Resource, { + return getResourceValueByJsonPath(obj, { jsonPathExpression: path, property: '', }); };docs/readme-generic-ui.md (2)
27-35: Clarify the purpose of both jsonPathExpression and property fields.The documentation explains what each field contains but doesn't clarify why both are needed. This has already been flagged in a past review comment. Consider adding an explanation that:
jsonPathExpressionis used at runtime to evaluate whether a resource is ready (JSONPath query evaluation)propertyis used for GraphQL field generation to ensure the necessary data is fetchedThis would help address the confusion raised in the previous review.
Example addition:
- Also `"resourceDefinition"` have optional field `readyCondition` that describing when resource treated as ready -It's an object that contain two fields. `jsonPathExpression` that contain JSONPath expression for complex data access and ready status matching and `property` JSON path to the resource property that contain info about resource ready state +It's an object that contains two fields: + - `jsonPathExpression`: JSONPath expression used to evaluate whether the resource is ready at runtime + - `property`: JSON path(s) used to generate GraphQL fields to fetch the necessary data for readiness evaluation
27-28: Fix markdown list indentation.Markdownlint reports incorrect indentation (4 spaces instead of 2).
Apply this diff:
- - in the `"resourceDefinition"` the given fields need to be specified: `group, plural, singular, kind, scope, namespace` describing properties of the resource. - - Also `"resourceDefinition"` have optional field `readyCondition` that describing when resource treated as ready + - in the `"resourceDefinition"` the given fields need to be specified: `group, plural, singular, kind, scope, namespace` describing properties of the resource. + - Also `"resourceDefinition"` have optional field `readyCondition` that describing when resource treated as readyprojects/wc/src/app/components/generic-ui/list-view/list-view.component.spec.ts (2)
430-470: Clarify test name to avoid confusion.The test name says "when Ready condition is missing" but the resource actually has a
Readycondition (withstatus: 'False'). What's missing is thereadyConditionconfiguration in theresourceDefinition.Consider renaming to:
"should mark resource as ready when readyCondition configuration is missing"to more accurately reflect what's being tested (the default behavior when no readyCondition is configured).
472-520: Clarify test name for precision.The test name says "when status condition is missing" but the resource does have a status condition (
type: 'Other'). What's missing is the specificReadycondition that thereadyConditionJSONPath is looking for.Consider renaming to:
"should mark resource as not ready when matching Ready status condition is not found"for clarity.projects/wc/src/app/components/generic-ui/list-view/list-view.component.ts (1)
240-249: Consider removing or changing console.log to console.debug.The readiness logic is correct, but line 247 contains
console.log('readyStatus', readyStatus)which should likely beconsole.debugor removed for production code to reduce console noise.Apply this diff:
- console.log('readyStatus', readyStatus); + console.debug('readyStatus', readyStatus);Or remove it entirely if the logging is no longer needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
docs/readme-generic-ui.md(2 hunks)package.json(2 hunks)projects/lib/models/models/resource.ts(2 hunks)projects/lib/portal-options/models/luigi-context.ts(1 hunks)projects/lib/portal-options/services/header-bar-renderers/namespace-selection-renderer.service.ts(2 hunks)projects/lib/portal-options/services/node-context-processing.service.spec.ts(2 hunks)projects/lib/services/resource/resource-node-context.ts(2 hunks)projects/lib/utils/utils/columns-to-gql-fields.spec.ts(1 hunks)projects/lib/utils/utils/columns-to-gql-fields.ts(1 hunks)projects/lib/utils/utils/get-value-by-path.ts(1 hunks)projects/lib/utils/utils/resource-field-by-path.spec.ts(1 hunks)projects/wc/src/app/components/dynamic-select/dynamic-select.component.ts(1 hunks)projects/wc/src/app/components/generic-ui/list-view/create-resource-modal/create-resource-modal.component.spec.ts(1 hunks)projects/wc/src/app/components/generic-ui/list-view/create-resource-modal/create-resource-modal.component.ts(1 hunks)projects/wc/src/app/components/generic-ui/list-view/delete-resource-confirmation-modal/delete-resource-modal.component.ts(1 hunks)projects/wc/src/app/components/generic-ui/list-view/list-view.component.spec.ts(3 hunks)projects/wc/src/app/components/generic-ui/list-view/list-view.component.ts(4 hunks)projects/wc/src/app/components/organization-management/organization-management.component.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
projects/lib/services/resource/resource-node-context.ts (1)
projects/lib/models/models/resource.ts (1)
ResourceDefinition(61-73)
projects/wc/src/app/components/generic-ui/list-view/list-view.component.ts (3)
projects/lib/utils/utils/columns-to-gql-fields.ts (1)
generateGraphQLFields(3-13)projects/lib/models/models/resource.ts (1)
Resource(53-59)projects/lib/utils/utils/resource-field-by-path.ts (1)
getResourceValueByJsonPath(4-22)
🪛 markdownlint-cli2 (0.18.1)
docs/readme-generic-ui.md
27-27: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
28-28: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pipe / build / build
🔇 Additional comments (24)
projects/wc/src/app/components/generic-ui/list-view/delete-resource-confirmation-modal/delete-resource-modal.component.ts (1)
18-18: Import path updated to align with PR objectives.The Resource import has been correctly migrated from the openmfp package to
@platform-mesh/portal-ui-lib/models, aligning with the PR's cleanup goals. All usages of the Resource type throughout the component remain unchanged and functional.projects/lib/utils/utils/get-value-by-path.ts (1)
2-2: LGTM! Import migration aligns with PR objectives.The import source update from
@openmfp/portal-ui-libto@platform-mesh/portal-ui-lib/modelscorrectly addresses the stated objective to clean up resource imports from openmfp.projects/wc/src/app/components/generic-ui/list-view/create-resource-modal/create-resource-modal.component.spec.ts (1)
5-5: Import path update is verified and consistent with codebase standards.The import path change from
@openmfp/portal-ui-libto@platform-mesh/portal-ui-lib/modelsis confirmed valid. This package is declared as a dependency inpackage.json, and the same import pattern is already used in 22 other files across the codebase, indicatingFieldDefinitionis properly exported from this module path.projects/wc/src/app/components/organization-management/organization-management.component.ts (3)
3-13: LGTM - Import formatting improvementThe multi-line formatting with trailing commas improves readability and makes diffs cleaner for future changes.
15-20: LGTM - Consistent formattingConsistent with the Angular core imports formatting.
35-42: LGTM - Consistent formattingUI5 webcomponents imports follow the same multi-line formatting pattern.
projects/lib/utils/utils/columns-to-gql-fields.spec.ts (1)
2-2: LGTM! Clean import path migration.The import path update for
FieldDefinitionaligns with the package migration from@openmfp/portal-ui-libto@platform-mesh/portal-ui-lib/models. No functional changes to tests.projects/lib/utils/utils/resource-field-by-path.spec.ts (1)
2-2: LGTM! Import path migration consistent with codebase changes.The updated import path for
FieldDefinitionandResourcealigns with the broader migration effort. No test behavior changes.package.json (1)
32-32: LGTM! Version bump supports module path migration.The minor version update of
@openmfp/portal-ui-libto 0.182.0 enables the new module paths (@platform-mesh/portal-ui-lib/models) used throughout this PR.projects/lib/utils/utils/columns-to-gql-fields.ts (1)
1-1: LGTM! Import path updated correctly.The
FieldDefinitionimport has been migrated to@platform-mesh/portal-ui-lib/modelswith no changes to the function logic.projects/lib/portal-options/models/luigi-context.ts (2)
2-2: LGTM! New import supports interface extension.The
Resourceimport from@platform-mesh/portal-ui-lib/modelsenables the newentityproperty onPortalNodeContext.
19-19: LGTM! Backward-compatible interface extension.Adding the optional
entity?: Resourceproperty toPortalNodeContextextends the context capabilities without breaking existing implementations.projects/lib/portal-options/services/node-context-processing.service.spec.ts (1)
5-8: LGTM! Import paths updated consistently.The imports have been migrated to the new package paths:
Resourcefrom@platform-mesh/portal-ui-lib/modelsResourceServicefrom@platform-mesh/portal-ui-lib/servicesNo test logic changes, maintaining existing test coverage.
projects/lib/services/resource/resource-node-context.ts (2)
2-2: LGTM! Import enables resourceDefinition context.The
ResourceDefinitionimport from@platform-mesh/portal-ui-lib/modelssupports the new context property, which includes thereadyConditionfield added in this PR.
5-5: LGTM! Backward-compatible interface extension.Adding the optional
resourceDefinition?: ResourceDefinitionproperty enables resource definition context (includingreadyCondition) to be passed through the node context without breaking existing implementations.projects/wc/src/app/components/generic-ui/list-view/create-resource-modal/create-resource-modal.component.ts (1)
23-23: LGTM! Import path migration complete.The
FieldDefinitionandResourcetypes have been migrated to@platform-mesh/portal-ui-lib/models, aligning with the package restructuring. No component logic changes.projects/wc/src/app/components/dynamic-select/dynamic-select.component.ts (1)
3-7: LGTM! Import path migration is clean.The import path updates from
@openmfp/portal-ui-libto@platform-mesh/portal-ui-libare consistent with the PR objectives.projects/lib/models/models/resource.ts (1)
68-71: LGTM! readyCondition field is well-structured.The optional
readyConditionfield follows the established pattern of combiningjsonPathExpressionfor complex data access withpropertyfor fallback/GraphQL field generation, consistent withFieldDefinition.projects/lib/portal-options/services/header-bar-renderers/namespace-selection-renderer.service.ts (1)
9-13: LGTM! Import migration is consistent.The import path updates align with the broader refactoring to use
@platform-mesh/portal-ui-lib.projects/wc/src/app/components/generic-ui/list-view/list-view.component.spec.ts (2)
382-428: LGTM! Test correctly validates readiness with False status.The test properly verifies that a resource with
Readycondition status ofFalseis marked as not ready whenreadyConditionis configured.
522-634: LGTM! Comprehensive test coverage for edge cases.The tests for empty conditions and mixed ready statuses provide good coverage of various readiness scenarios.
projects/wc/src/app/components/generic-ui/list-view/list-view.component.ts (3)
104-104: LGTM! Clean computed property.The
readyConditioncomputed property properly derives fromresourceDefinition()?.readyCondition.
120-139: LGTM! Core readyCondition integration is well-implemented.The changes properly integrate readyCondition by:
- Using
generateGqlFieldsWithReadyConditions()to include readyCondition fields in GraphQL queries (line 120)- Dynamically determining readiness with
getResourceReadyStatus(resource)instead of hardcodedtrue(line 133)
231-238: LGTM! GraphQL field generation handles readyCondition correctly.The method appropriately concatenates
readyConditionwith columns when present, treating it as aFieldDefinition-like object for GraphQL field generation.
Cleanup resource.ts import from openmfp
Add readyCondition to resourceDefintion
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation