Fix InfraEnv creation in single-cluster mode#3266
Fix InfraEnv creation in single-cluster mode#3266zaneb wants to merge 2 commits intoopenshift-assisted:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zaneb The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughReplaces conditional infra-env creation with a new InfraEnvsService.getOrCreate(...). ClustersService and useInfraEnvId call getOrCreate when a pullSecret is present; when no pullSecret, useInfraEnvId calls getInfraEnvId(...) and propagates errors. Test interceptors updated to simulate POST-driven infra-env creation state. Changes
Sequence Diagram(s)sequenceDiagram
participant Hook as useInfraEnvId
participant InfraSvc as InfraEnvsService
participant API as InfraEnvsAPI
Note over Hook,InfraSvc: Decision based on presence of pullSecret
alt pullSecret present
Hook->>InfraSvc: getOrCreate(params, isSingleClusterFeatureEnabled)
InfraSvc->>InfraSvc: validate clusterId & cpuArchitecture
InfraSvc->>InfraSvc: getInfraEnvId(cpuArch, flag)
alt infraEnvId found
InfraSvc->>API: get(infraEnvId)
API-->>InfraSvc: InfraEnv
InfraSvc-->>Hook: return InfraEnv (cache updated)
else infraEnvId missing or 404
InfraSvc->>API: create(params)
API-->>InfraSvc: created InfraEnv
InfraSvc-->>Hook: return created InfraEnv (cache updated)
end
else no pullSecret
Hook->>InfraSvc: getInfraEnvId(cpuArch, flag)
InfraSvc-->>Hook: infraEnvId or Error (propagate)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
openshift-ci
bot
added
the
size/M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libs/ui-lib/lib/ocm/services/InfraEnvsService.ts (1)
85-92: Consider distinguishing between "not found" and other errors.Currently, any
Errorreturned bygetInfraEnvIdis treated as "InfraEnv not found" and triggers creation. However, the error could represent a network failure, permission issue, or other problems that shouldn't automatically fall through to creation.Consider logging or handling unexpected errors before falling back to creation:
if (existingInfraEnvId && !(existingInfraEnvId instanceof Error)) { // InfraEnv exists, fetch and return it const { data: infraEnv } = await InfraEnvsAPI.get(existingInfraEnvId); return infraEnv; + } else if (existingInfraEnvId instanceof Error) { + // Log the error for observability, but proceed with creation + console.warn('Error checking for existing InfraEnv:', existingInfraEnvId); } // No InfraEnv exists, create one return InfraEnvsService.create(params);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts(1 hunks)libs/ui-lib/lib/ocm/services/ClustersService.ts(1 hunks)libs/ui-lib/lib/ocm/services/InfraEnvsService.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
libs/ui-lib/lib/ocm/services/InfraEnvsService.ts (1)
libs/types/assisted-installer-service.d.ts (3)
InfraEnvCreateParams(1828-1872)InfraEnv(1750-1827)Error(1001-1022)
libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts (1)
libs/types/assisted-installer-service.d.ts (2)
InfraEnvCreateParams(1828-1872)Error(1001-1022)
⏰ 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). (6)
- GitHub Check: translation-files
- GitHub Check: tests
- GitHub Check: unit-tests
- GitHub Check: format
- GitHub Check: circular-deps
- GitHub Check: lint
🔇 Additional comments (2)
libs/ui-lib/lib/ocm/services/ClustersService.ts (1)
51-51: LGTM - Proper integration with newgetOrCreatemethod.The change correctly delegates InfraEnv creation/retrieval to
InfraEnvsService.getOrCreate, passing both the parameters and theisSingleClusterFeatureEnabledflag. This ensures InfraEnvs are created when needed in single-cluster mode, as intended by the PR objectives.libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts (1)
25-47: LGTM - Proper handling of both creation and retrieval paths.The conditional logic correctly handles two scenarios:
- When
pullSecretis available: usesgetOrCreateto ensure an InfraEnv exists (creating if needed)- When
pullSecretis unavailable: falls back to searching for an existing InfraEnv onlyThis aligns with the PR objective where pullSecret is available from the form in single-cluster mode, enabling proper InfraEnv creation during cluster setup.
zaneb
force-pushed
the
fix-create-infraenv
branch
2 times, most recently
from
4c75f11 to
0021993
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/ui-lib/lib/ocm/services/InfraEnvsService.ts (1)
97-103: Consider refining error handling to avoid masking real issues.The catch-all error handling and string matching on
error.message(line 99) could mask authentication or network failures. When such errors occur, the code falls through tocreate(), which will likely fail with the same error.Consider being more selective:
- Only catch and suppress "not found" scenarios (expected when no InfraEnv exists)
- Let authentication, network, and other unexpected errors propagate to the caller
- If the API returns structured errors with a
codefield (per the Error type), check that instead of the message stringExample approach:
try { const existingInfraEnvId = await InfraEnvsService.getInfraEnvId( params.clusterId, params.cpuArchitecture, isSingleClusterFeatureEnabled, ); if (existingInfraEnvId && !(existingInfraEnvId instanceof Error)) { const { data: infraEnv } = await InfraEnvsAPI.get(existingInfraEnvId); InfraEnvCache.updateInfraEnvs(params.clusterId, [infraEnv]); return infraEnv; } } catch (error) { - // Log unexpected errors before falling back to creation - if (error instanceof Error && !error.message.includes('not found')) { - console.warn('Unexpected error while fetching InfraEnv, will attempt creation:', error); - } - // Fall through to create a new InfraEnv + // Only suppress expected "not found" errors; re-throw others + if (error instanceof Error && error.message.includes('not found')) { + // Fall through to create a new InfraEnv + } else { + throw error; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts(1 hunks)libs/ui-lib/lib/ocm/services/ClustersService.ts(1 hunks)libs/ui-lib/lib/ocm/services/InfraEnvsService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/ui-lib/lib/ocm/services/ClustersService.ts
🧰 Additional context used
🧬 Code graph analysis (2)
libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts (1)
libs/types/assisted-installer-service.d.ts (2)
InfraEnvCreateParams(1828-1872)Error(1001-1022)
libs/ui-lib/lib/ocm/services/InfraEnvsService.ts (1)
libs/types/assisted-installer-service.d.ts (3)
InfraEnvCreateParams(1828-1872)InfraEnv(1750-1827)Error(1001-1022)
⏰ 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). (6)
- GitHub Check: circular-deps
- GitHub Check: tests
- GitHub Check: unit-tests
- GitHub Check: translation-files
- GitHub Check: lint
- GitHub Check: format
🔇 Additional comments (3)
libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts (1)
25-46: LGTM! Clear separation of pullSecret vs. no-pullSecret workflows.The refactored logic correctly implements the PR objectives:
- When
pullSecretis available (lines 25-36),getOrCreateensures an InfraEnv is created or retrieved with all necessary parameters.- When
pullSecretis absent (lines 37-46), the code retrieves an existing InfraEnv without attempting creation, supporting the late-binding workflow.This separation fixes the issue where InfraEnv creation was unconditionally skipped in single-cluster mode.
libs/ui-lib/lib/ocm/services/InfraEnvsService.ts (2)
78-80: Validation addresses previous concern.The cpuArchitecture validation correctly resolves the past review comment about handling the optional parameter. Since
getInfraEnvId(line 84-87) requires a non-optionalcpuArchitecture, this guard ensures type safety and prevents runtime errors.
90-96: Cache update addresses previous concern.The cache update at lines 93-94 correctly resolves the past review comment about maintaining cache consistency. Fetched InfraEnvs are now cached just like created ones (line 66), preventing unnecessary API calls.
zaneb
force-pushed
the
fix-create-infraenv
branch
from
0021993 to
57a70e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts (1)
37-46: LGTM: Correct fallback for missing pullSecret.The else branch correctly handles the case when
pullSecretis unavailable by attempting to find an existing InfraEnv without creating a new one. This aligns with the PR objectives for the late-binding workflow.The
instanceof Errorcheck on line 44 is a defensive pattern to handle the API's return type. While this pattern works, consider whether the service layer should adopt a more conventional approach (throwing errors instead of returning them) for consistency across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts(1 hunks)libs/ui-lib/lib/ocm/services/ClustersService.ts(1 hunks)libs/ui-lib/lib/ocm/services/InfraEnvsService.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/ui-lib/lib/ocm/services/ClustersService.ts
- libs/ui-lib/lib/ocm/services/InfraEnvsService.ts
🧰 Additional context used
🧬 Code graph analysis (1)
libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts (1)
libs/types/assisted-installer-service.d.ts (2)
InfraEnvCreateParams(1828-1872)Error(1001-1022)
| if (pullSecret) { | ||
| const infraEnv = await InfraEnvsService.getOrCreate( | ||
| { | ||
| name: InfraEnvsService.makeInfraEnvName(cpuArchitecture, clusterName), | ||
| pullSecret, | ||
| clusterId, | ||
| openshiftVersion, | ||
| cpuArchitecture: cpuArchitecture as InfraEnvCreateParams['cpuArchitecture'], | ||
| }); | ||
| setInfraEnv(infraEnv.id); | ||
| }, | ||
| isSingleClusterFeatureEnabled, | ||
| ); | ||
| setInfraEnv(infraEnv.id); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the type assertion for cpuArchitecture.
The logic correctly uses getOrCreate when pullSecret is available, which aligns with the PR objectives. However, the type assertion on line 32 should be verified to ensure type safety.
Run the following script to verify that the CpuArchitecture type matches the expected union type:
🏁 Script executed:
#!/bin/bash
# Description: Verify CpuArchitecture type definition matches InfraEnvCreateParams['cpuArchitecture']
# Search for CpuArchitecture type definition
echo "=== Searching for CpuArchitecture type definition ==="
ast-grep --pattern 'type CpuArchitecture = $$$'
echo ""
echo "=== Searching for CpuArchitecture export ==="
rg -nP --type=ts "export (type|interface) CpuArchitecture" -A 3
echo ""
echo "=== Expected type from InfraEnvCreateParams ==="
echo "cpuArchitecture?: 'x86_64' | 'aarch64' | 'arm64' | 'ppc64le' | 's390x'"Length of output: 549
🏁 Script executed:
#!/bin/bash
# Description: Search for CpuArchitecture type and InfraEnvCreateParams definitions
echo "=== Full content of cpuArchitecture.ts ==="
cat libs/ui-lib/lib/common/types/cpuArchitecture.ts
echo ""
echo "=== Search for InfraEnvCreateParams definition ==="
rg -nP --type=ts "InfraEnvCreateParams" -A 5 -B 1 | head -50
echo ""
echo "=== Search for cpuArchitecture import/usage in useInfraEnvId.ts ==="
rg -nP --type=ts "cpuArchitecture" libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts -B 2 -A 2Length of output: 8339
🏁 Script executed:
#!/bin/bash
# Description: Find where useInfraEnvId is called and what values are passed
echo "=== Search for useInfraEnvId call sites ==="
rg -nP --type=ts "useInfraEnvId\(" -B 2 -A 3 | head -100
echo ""
echo "=== Check if SupportedCpuArchitecture is used elsewhere for similar pattern ==="
rg -nP --type=ts "SupportedCpuArchitecture" libs/ui-lib/lib/ocm/ -B 1 -A 1 | head -50
echo ""
echo "=== Check InfraEnvsService.getOrCreate implementation ==="
rg -nP --type=ts "async getOrCreate" libs/ui-lib/lib/ocm/services/InfraEnvsService.ts -A 20 | head -40Length of output: 5948
Narrow the cpuArchitecture parameter type or add runtime validation before the type assertion.
The type assertion on line 32 is unsafe. The function parameter accepts the full CpuArchitecture enum (which includes 'multi' and 'cluster-day1-cpu-architecture'), but casts it to InfraEnvCreateParams['cpuArchitecture'] (which only accepts 'x86_64' | 'aarch64' | 'arm64' | 'ppc64le' | 's390x'). If invalid architecture values are passed, they will bypass the cast and cause runtime errors at the API layer.
Either:
- Change the parameter type to
SupportedCpuArchitectureor create a narrower type that matchesInfraEnvCreateParams['cpuArchitecture'], or - Add validation in
getOrCreate(lines 79–81 ofInfraEnvsService.ts) to reject invalid values before use.
Note: The same pattern exists in ClustersService.ts line 43 and should also be addressed.
🤖 Prompt for AI Agents
In libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts around lines 25 to 36, the
cpuArchitecture is being unsafely asserted to
InfraEnvCreateParams['cpuArchitecture']; narrow the accepted type or validate at
runtime to prevent invalid values (like 'multi' or
'cluster-day1-cpu-architecture') reaching the API. Fix by changing the hook
parameter type to a SupportedCpuArchitecture (matching
InfraEnvCreateParams['cpuArchitecture']) or add a runtime guard that maps/throws
for unsupported enum values before calling InfraEnvsService.getOrCreate; also
apply the same type narrowing or validation in ClustersService.ts at line 43 to
keep behavior consistent.
57a70e3 to
504542d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts (1)
25-45: New get‑or‑create flow matches the intended behavior, but cpuArchitecture typing remains unsafe.The split logic looks good:
- With
pullSecret, delegating toInfraEnvsService.getOrCreate(…, isSingleClusterFeatureEnabled)and settinginfraEnv.idis exactly what the single‑cluster + late‑binding flows need.- Without
pullSecret, falling back togetInfraEnvId(lookup‑only, no creation) is a sensible and safer default.Two concerns remain:
Unsafe
cpuArchitectureassertion (previously flagged).
cpuArchitectureis still aCpuArchitecture, but it’s asserted toInfraEnvCreateParams['cpuArchitecture']:cpuArchitecture: cpuArchitecture as InfraEnvCreateParams['cpuArchitecture'],Since
CpuArchitectureincludes values that are not in the allowed set forInfraEnvCreateParams['cpuArchitecture'](e.g.'multi','cluster-day1-cpu-architecture'), this cast can let invalid values reach the API. Please either narrow the hook parameter type to the supported subset (e.g. aSupportedCpuArchitectureunion that matchesInfraEnvCreateParams['cpuArchitecture']) or add a runtime guard that rejects/masks unsupported values before callinggetOrCreate. This is the same issue mentioned in a past review and still needs resolution.
instanceof Errorcheck may be obsolete or ineffective.
In the no‑pullSecret branch:if (infraEnvId && !(infraEnvId instanceof Error)) { setInfraEnv(infraEnvId); }If
getInfraEnvIdnow throws on non‑404 errors (per the service changes) or returns plain API error objects, this guard will either never see anErrorinstance or never be reached. Consider tightening thegetInfraEnvIdreturn type so it’s juststring | undefined, then drop theinstanceofcheck here, or add an explicit runtime discriminant (e.g.'kind' === 'Error') if API error objects are intentionally returned.Overall behavior is solid; these are mainly type‑safety and cleanup issues.
🧹 Nitpick comments (1)
libs/ui-lib-tests/cypress/support/interceptors.ts (1)
261-265: Day‑1 infra‑env filter intercept correctly simulates “none found”; consider loosening URL match.Returning an empty array here is aligned with the new
InfraEnvsService.getOrCreatebehavior and will correctly drive the “create on missing” path during cluster creation. The change looks good.If you expect additional query params (e.g. pagination) or different param ordering in future, it may be safer to intercept with a wildcard (like the DAY2 flow does) and branch on
req.query.cluster_idinstead of hard‑coding the full?cluster_id=${Cypress.env('clusterId')}string.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/ui-lib-tests/cypress/support/interceptors.ts(1 hunks)libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts(1 hunks)libs/ui-lib/lib/ocm/services/ClustersService.ts(1 hunks)libs/ui-lib/lib/ocm/services/InfraEnvsService.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/ui-lib/lib/ocm/services/InfraEnvsService.ts
- libs/ui-lib/lib/ocm/services/ClustersService.ts
🧰 Additional context used
🧬 Code graph analysis (1)
libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts (1)
libs/types/assisted-installer-service.d.ts (2)
InfraEnvCreateParams(1828-1872)Error(1001-1022)
⏰ 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). (6)
- GitHub Check: unit-tests
- GitHub Check: tests
- GitHub Check: translation-files
- GitHub Check: circular-deps
- GitHub Check: lint
- GitHub Check: format
zaneb
force-pushed
the
fix-create-infraenv
branch
from
504542d to
ba8d1f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts (1)
25-36: Approve the getOrCreate logic; type assertion already flagged.The switch to
InfraEnvsService.getOrCreatecorrectly implements the PR objective to ensure InfraEnv creation in single-cluster mode whenpullSecretis provided. The method will handle both retrieval and creation seamlessly.However, the type assertion on line 32 remains unsafe (as noted in previous reviews) -
CpuArchitectureincludes values like'multi'and'cluster-day1-cpu-architecture'that are invalid forInfraEnvCreateParams['cpuArchitecture'].Verify that
getOrCreatehandles all the creation parameters correctly and returns a valid InfraEnv:#!/bin/bash # Description: Verify getOrCreate implementation and parameter handling echo "=== Check getOrCreate implementation ===" ast-grep --pattern 'async getOrCreate($$$) { $$$ }' echo "" echo "=== Verify getOrCreate return type and error handling ===" rg -nP --type=ts "getOrCreate.*:" libs/ui-lib/lib/ocm/services/InfraEnvsService.ts -A 30
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/ui-lib-tests/cypress/support/interceptors.ts(1 hunks)libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts(1 hunks)libs/ui-lib/lib/ocm/services/ClustersService.ts(1 hunks)libs/ui-lib/lib/ocm/services/InfraEnvsService.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/ui-lib/lib/ocm/services/InfraEnvsService.ts
- libs/ui-lib/lib/ocm/services/ClustersService.ts
- libs/ui-lib-tests/cypress/support/interceptors.ts
🧰 Additional context used
🧬 Code graph analysis (1)
libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts (1)
libs/types/assisted-installer-service.d.ts (2)
InfraEnvCreateParams(1828-1872)Error(1001-1022)
⏰ 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). (6)
- GitHub Check: tests
- GitHub Check: translation-files
- GitHub Check: circular-deps
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: format
Introduces a new InfraEnvsService.getOrCreate() method that encapsulates the common pattern of checking if an InfraEnv exists and creating one if it doesn't. This eliminates code duplication and provides a single place for this logic. The method: - Checks if an InfraEnv already exists for the cluster - Returns the existing InfraEnv if found - Creates a new InfraEnv if none exists Updated useInfraEnvId hook to use this new method, simplifying the code and making the intent clearer. This is a pure refactoring with no behavior change. Assisted-by: Claude Code Signed-off-by: Zane Bitter <zbitter@redhat.com>
zaneb
force-pushed
the
fix-create-infraenv
branch
4 times, most recently
from
1f0fff0 to
f55950e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/ui-lib-tests/cypress/support/interceptors.ts (1)
249-279: Day1 infra-env intercept state handling looks correct; consider tightening signal checkThe new
infraEnvCreatedflag and the GET/POST intercepts nicely model the "get or create" behavior: first GET returns[], POST setsinfraEnvCreated = true, subsequent GETs return[baseInfraEnv], and you also allow pre-existing infra-envs via theAI_LAST_SIGNALpath.Two small suggestions:
- To make intent clearer, you could normalize
signalIndicatesCreatedto a strict boolean, e.g.:const currentSignal = Cypress.env('AI_LAST_SIGNAL') as string | undefined; const signalIndicatesCreated = !!currentSignal;
- Confirm that "any non-empty
AI_LAST_SIGNAL" is really the right proxy for "infra-env already exists". If a future scenario uses signals that shouldn’t imply an existing infra-env, a more specific check (e.g., dedicated signal or helper) might avoid surprising test behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/ui-lib-tests/cypress/support/interceptors.ts(2 hunks)libs/ui-lib/lib/ocm/services/ClustersService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/ui-lib/lib/ocm/services/ClustersService.ts
⏰ 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). (6)
- GitHub Check: tests
- GitHub Check: unit-tests
- GitHub Check: translation-files
- GitHub Check: circular-deps
- GitHub Check: lint
- GitHub Check: format
zaneb
force-pushed
the
fix-create-infraenv
branch
from
f55950e to
e9c9abd
Compare
openshift-ci
bot
added
size/L
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/ui-lib-tests/cypress/support/interceptors.ts (1)
263-276: Simplify the signal check for better readability.The dynamic GET interceptor correctly implements the state-based behavior needed for the
getOrCreateflow. However, the condition on line 267 can be simplified.Apply this diff to simplify the truthy check:
- const willReturnFixture = infraEnvCreated || (currentSignal !== '' && currentSignal !== undefined && currentSignal !== null); + const willReturnFixture = infraEnvCreated || !!currentSignal;This achieves the same result since JavaScript's double-negation (
!!) coerces empty strings,undefined, andnulltofalse.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/ui-lib-tests/cypress/support/interceptors.ts(2 hunks)libs/ui-lib/lib/ocm/services/ClustersService.ts(1 hunks)
⏰ 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). (6)
- GitHub Check: format
- GitHub Check: tests
- GitHub Check: unit-tests
- GitHub Check: translation-files
- GitHub Check: lint
- GitHub Check: circular-deps
🔇 Additional comments (2)
libs/ui-lib/lib/ocm/services/ClustersService.ts (1)
51-51: LGTM! Delegation togetOrCreatefixes the single-cluster workflow.The unconditional call to
InfraEnvsService.getOrCreate()ensures that:
- In normal single-cluster mode with pullSecret, an InfraEnv is created during cluster creation
- In late-binding scenarios, an existing unbound InfraEnv is reused
- The creation/retrieval logic is properly encapsulated in the service layer
Not capturing the return value is appropriate since the infraEnv can be retrieved later via the clusterId relationship.
libs/ui-lib-tests/cypress/support/interceptors.ts (1)
278-282: LGTM! POST interceptor correctly updates creation state.The interceptor properly sets the
infraEnvCreatedflag before delegating tomockInfraEnvResponse, ensuring that subsequent GET requests return the fixture instead of an empty array. The diagnostic logging aids in test debugging.
After commit 118a92b, InfraEnv creation was unconditionally skipped when the single-cluster feature flag was enabled. This broke the existing single-cluster UI workflow because: 1. No InfraEnv was created during cluster creation 2. The useInfraEnvId hook tried to create one later, but failed because pullSecret is unavailable in single-cluster mode (no OCM integration) 3. The UI could not proceed without an InfraEnv Fix by using InfraEnvsService.getOrCreate() in ClustersService.create() instead of conditionally skipping creation. This ensures: - Normal single-cluster UI: Creates InfraEnv during cluster creation when pullSecret is available from the form - Late-binding workflow: Reuses externally-created unbound InfraEnv when one exists Assisted-by: Claude Code Signed-off-by: Zane Bitter <zbitter@redhat.com>
zaneb
force-pushed
the
fix-create-infraenv
branch
from
e9c9abd to
d787442
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/ui-lib-tests/cypress/support/interceptors.ts (1)
251-282: Day1 infra-env intercept flow looks correct; consider tightening the signal heuristicThe new
infraEnvCreatedflag and GET/POST intercepts nicely model thegetOrCreatebehavior (empty list before POST, fixture after POST or for pre-existing infra-env), which should unblock the single-cluster flow in tests.One subtle point:
willReturnFixtureis true for any non-emptyAI_LAST_SIGNAL, effectively treating all wizard signals as “infraEnv already exists”. If in future some scenarios setAI_LAST_SIGNALbefore an infra-env actually exists, tests could mask bugs by returning a fixture too early.If feasible, consider:
- Using
hasWizardSignal('<specific-signal-that-means-infraenv-exists>')instead ofcurrentSignal !== '', or- Restricting the check to a small set of known signals that truly imply “infra-env present”.
This keeps the test behavior aligned with real lifecycle semantics while preserving your new flow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/ui-lib-tests/cypress/support/interceptors.ts(2 hunks)libs/ui-lib/lib/ocm/services/ClustersService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/ui-lib/lib/ocm/services/ClustersService.ts
⏰ 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). (6)
- GitHub Check: unit-tests
- GitHub Check: circular-deps
- GitHub Check: tests
- GitHub Check: translation-files
- GitHub Check: lint
- GitHub Check: format
|
This is probably moot now that #3259 has merged. We should be able to fix the UI by merging openshift/installer#9960 at this point. |
|
@zaneb: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
1 participant
After commit 118a92b, InfraEnv creation was unconditionally skipped when
the single-cluster feature flag was enabled. This broke the existing
single-cluster UI workflow because:
pullSecret is unavailable in single-cluster mode (no OCM integration)
Fix by using InfraEnvsService.getOrCreate() in ClustersService.create()
instead of conditionally skipping creation. This ensures:
pullSecret is available from the form
one exists
Summary by CodeRabbit
New Features
Refactor
Compatibility
Tests