Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end support for NamespacedCloudProfiles across backend fixtures, services, routes, authorization, cache and hooks; frontend store, composables, components, router guards and tests; introduces jsondiffpatch mocks, Vitest config, kube-client resource, RBAC, and ZIP/runtime-related .pnp.cjs updates noted in runtime state. Changes
Sequence Diagram(s)sequenceDiagram
participant Frontend as Frontend
participant Backend as Backend
participant Cache as Cache
participant KubeAPI as KubeAPI
Frontend->>Backend: GET /api/namespaces/{ns}/namespacedcloudprofiles?diff=true
Backend->>Backend: canListNamespacedCloudProfiles(user, ns)
Backend->>KubeAPI: SelfSubjectAccessReview (list namespacedcloudprofiles)
KubeAPI-->>Backend: allowed / denied
alt allowed
Backend->>Cache: getNamespacedCloudProfiles(ns)
Cache-->>Backend: namespaced profiles (possibly diffs)
Backend->>Cache: getCloudProfile(parentName) for diffs
Cache-->>Backend: parent CloudProfile
Backend->>Backend: compute/rehydrate using jsondiffpatch
Backend-->>Frontend: 200 JSON list (simplified or diffed)
else denied
Backend-->>Frontend: 403 Forbidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (12)
backend/rollup.config.js (1)
28-28: Consider makingsourcemapconditional on the build environment.
sourcemap: trueis unconditionally enabled, so.mapfiles are emitted for every build including production. For a backend bundle this is low-risk (the files aren't HTTP-served), but it adds unnecessary disk/artifact overhead in production and exposes original source paths if thedistfolder is ever inadvertently shared.♻️ Proposed change to gate sourcemaps on the environment
+const isDev = process.env.NODE_ENV !== 'production' + export default [ { ... output: { ... - sourcemap: true, + sourcemap: isDev, ... }, }, ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/rollup.config.js` at line 28, sourcemap is always true in the Rollup config; change it to be conditional on the build environment by introducing an isProduction flag (e.g., derived from process.env.NODE_ENV or a build-mode variable) and set the Rollup output.sourcemap to false when isProduction is true and true otherwise; update the rollup config where the sourcemap property is set (the output.sourcemap entry) so production builds skip emitting .map files while dev builds keep them.frontend/src/utils/index.js (1)
159-177:getCloudProfileSpecimplementation is correct; minor redundancy in optional chaining.Inside the
if (cloudProfile?.kind === 'NamespacedCloudProfile')branch,cloudProfileis already guaranteed to be non-nullish (a nullishcloudProfilewould yieldundefinedfor?.kind, which is never=== 'NamespacedCloudProfile'). ThecloudProfile?.on line 174 can safely becloudProfile.:✨ Optional cleanup
export function getCloudProfileSpec (cloudProfile) { if (cloudProfile?.kind === 'NamespacedCloudProfile') { - return cloudProfile?.status?.cloudProfileSpec ?? {} + return cloudProfile.status?.cloudProfileSpec ?? {} } return cloudProfile?.spec ?? {} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/utils/index.js` around lines 159 - 177, The optional chaining on cloudProfile in getCloudProfileSpec is redundant after checking cloudProfile?.kind; update the NamespacedCloudProfile branch to use cloudProfile.status?.cloudProfileSpec (i.e., remove the leading ?. before status) so we still guard status but avoid double-optional on cloudProfile; keep the fallback to {} unchanged.frontend/__tests__/utils/index.spec.js (1)
552-559: Consider adding an explicit test forstatuspresent butcloudProfileSpecabsent/null.The current "without status" test exercises
cloudProfile?.status?.cloudProfileSpecshort-circuiting on the missingstatus, which hits?? {}. A separate case wherestatusexists butcloudProfileSpecis missing ({ status: {} }) or explicitlynull({ status: { cloudProfileSpec: null } }) would independently confirm the??fallback and make the contract of the{}default explicit.💡 Suggested additional test case
it('should return empty object for NamespacedCloudProfile without status', () => { const cloudProfile = { kind: 'NamespacedCloudProfile', metadata: { name: 'custom' }, spec: { parent: { kind: 'CloudProfile', name: 'aws' } }, } expect(getCloudProfileSpec(cloudProfile)).toEqual({}) }) + + it('should return empty object for NamespacedCloudProfile with status but no cloudProfileSpec', () => { + const cloudProfile = { + kind: 'NamespacedCloudProfile', + metadata: { name: 'custom' }, + spec: { parent: { kind: 'CloudProfile', name: 'aws' } }, + status: {}, + } + expect(getCloudProfileSpec(cloudProfile)).toEqual({}) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/utils/index.spec.js` around lines 552 - 559, Add explicit unit tests covering the case where status exists but cloudProfileSpec is missing or null: in frontend/__tests__/utils/index.spec.js add two tests calling getCloudProfileSpec with a NamespacedCloudProfile that has metadata and spec but status: {} and another with status: { cloudProfileSpec: null }, and assert both return {} to verify the ?? fallback; reference the existing test for 'should return empty object for NamespacedCloudProfile without status' and replicate its structure but set status explicitly to {} and to { cloudProfileSpec: null } while keeping the expect(...).toEqual({}).frontend/src/composables/useCloudProfile/useMachineTypes.js (1)
70-71: Inconsistent spec access — preferget()with a default forregions.Line 70 uses direct
.regionsproperty access on the spec object, while every other access in this PR (including line 43 of this file anduseVolumeTypes.jsline 63) usesget()with a safe default. Although lodash'sfindtoleratesundefinedas the collection, the inconsistency is worth aligning.♻️ Suggested fix
- const regionObject = find(getCloudProfileSpec(cloudProfile.value).regions, { name: region }) + const regionObject = find(get(getCloudProfileSpec(cloudProfile.value), ['regions'], []), { name: region })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/composables/useCloudProfile/useMachineTypes.js` around lines 70 - 71, Replace the direct property access of regions on the spec with a safe lodash get; instead of calling find(getCloudProfileSpec(cloudProfile.value).regions, ...) use find(get(getCloudProfileSpec(cloudProfile.value), ['regions'], []), { name: region }) so regionObject and subsequently regionZones use a consistent safe default; update the references to getCloudProfileSpec, regionObject, regionZones, and find accordingly to match other files' pattern.frontend/src/composables/useShootHelper.js (1)
99-108: Variable shadowing:namespaceon line 102 shadows the outer computednamespacefrom line 49/72.The
namespacecomputed (fromshootPropertyMappings) is defined in the outer scope. Theconst namespaceinside thiscomputedcallback shadows it, which can be confusing during maintenance.♻️ Suggested rename for clarity
const defaultCloudProfileRef = computed(() => { const defaultCloudProfile = head(cloudProfiles.value) const name = get(defaultCloudProfile, ['metadata', 'name']) - const namespace = get(defaultCloudProfile, ['metadata', 'namespace']) + const profileNamespace = get(defaultCloudProfile, ['metadata', 'namespace']) return { name, - kind: namespace ? 'NamespacedCloudProfile' : 'CloudProfile', - ...(namespace && { namespace }), + kind: profileNamespace ? 'NamespacedCloudProfile' : 'CloudProfile', + ...(profileNamespace && { namespace: profileNamespace }), } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/composables/useShootHelper.js` around lines 99 - 108, The inner const named namespace inside the computed defaultCloudProfileRef shadows the outer computed namespace from shootPropertyMappings; rename the inner variable (e.g., profileNamespace or cpNamespace) and update its usage in the returned object (the kind check and optional namespace spread) so defaultCloudProfileRef uses the new name while leaving the outer namespace untouched; locate this change around the computed defaultCloudProfileRef function and adjust the references to get(defaultCloudProfile, ['metadata','namespace']) accordingly.backend/jest.globalSetup.cjs (1)
37-47:globalSetupis declaredasyncbut contains noawaitexpressions.Minor: the function doesn't use
await, soasyncis unnecessary. Not a bug since Jest supports both sync and asyncglobalSetup, but it's slightly misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/jest.globalSetup.cjs` around lines 37 - 47, The exported function globalSetup is declared async but never uses await; remove the unnecessary async keyword to make it a synchronous function instead (change "module.exports = async function globalSetup () {" to "module.exports = function globalSetup () {"), keeping the existing calls to shouldRebuildPackages() and execSync unchanged so behavior remains identical.backend/__fixtures__/index.cjs (1)
28-28: Inconsistent file extension in require path.All other fixture imports omit the file extension (e.g.,
require('./cloudprofiles')), but this one explicitly uses.cjs. For consistency, either drop the extension here or add it to all other requires.Proposed fix
-const namespacedcloudprofiles = require('./namespacedcloudprofiles.cjs') +const namespacedcloudprofiles = require('./namespacedcloudprofiles')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/__fixtures__/index.cjs` at line 28, Inconsistent require path: change the require call that assigns namespacedcloudprofiles (currently "require('./namespacedcloudprofiles.cjs')") to match the other fixtures by removing the ".cjs" extension (i.e., require('./namespacedcloudprofiles')), or alternatively add the ".cjs" extension to the other fixture requires for consistency; update the require expression that references namespacedcloudprofiles to the chosen consistent form so imports across the file follow the same pattern.backend/__fixtures__/namespacedcloudprofiles.cjs (1)
11-11: Nit:kindparameter name is overloaded and slightly misleading.The
kindparameter is used as the infrastructure provider type (local,infra1, etc.) forstatus.cloudProfileSpec.typeand machine type name prefixes, whilespec.parent.kindis hardcoded to'CloudProfile'. Consider renaming toproviderTypeorinfraTypefor clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/__fixtures__/namespacedcloudprofiles.cjs` at line 11, Rename the confusing parameter name "kind" in the getNamespacedCloudProfile function to a clearer name like "providerType" (or "infraType") and update all internal references where it is used to set status.cloudProfileSpec.type and to build machine type name prefixes; keep spec.parent.kind hardcoded as 'CloudProfile'. Specifically, change the parameter in getNamespacedCloudProfile and replace usages that assign to status.cloudProfileSpec.type and any prefix generation logic to use providerType, and update any call sites of getNamespacedCloudProfile to pass the renamed argument.frontend/src/store/cloudProfile/index.js (1)
138-156:cloudProfileByRef— name-only fallback may match wrong profile.Line 150 falls back to matching by
metadata.nameonly when the ref has nonamespace. If two namespaces contain profiles with the same name, this returns the first arbitrary match. The JSDoc documents this behavior, so it's an intentional trade-off.Consider logging when the fallback path is taken, to aid debugging ambiguous matches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/store/cloudProfile/index.js` around lines 138 - 156, The fallback in cloudProfileByRef that matches namespacedList by name-only can return an ambiguous entry; update cloudProfileByRef to emit a debug/warn log when the name-only fallback is used so callers can trace ambiguous matches—specifically, inside cloudProfileByRef when cloudProfileRef.kind === 'NamespacedCloudProfile' and cloudProfileRef.namespace is falsy (the branch that calls find(namespacedList.value, ['metadata.name', cloudProfileRef.name'])), add a clear log statement (using the existing store logger or console.warn) that includes cloudProfileRef.name and note that the namespace was missing and a name-only match was returned, referencing namespacedList and the find call for context.backend/__tests__/services.namespacedCloudProfiles.spec.js (2)
236-251: Forbidden test invokes the service twice with only one mock setup.Lines 242-244 and 246-248 each call
listForNamespace, consuming two auth checks total. Only onemockResolvedValueOnce(false)is configured (line 240); the second call falls through to the defaultvi.fn()return (undefined), which happens to be falsy and still triggersForbidden. This works by coincidence.Consider either adding a second mock setup or splitting into two separate
itblocks — one for the error type and one for the message.Proposed fix — single call verifying both type and message
- authorization.canListNamespacedCloudProfiles.mockResolvedValueOnce(false) - - await expect( - namespacedCloudProfiles.listForNamespace({ user, namespace }), - ).rejects.toThrow(Forbidden) - - await expect( - namespacedCloudProfiles.listForNamespace({ user, namespace }), - ).rejects.toThrow(`You are not allowed to list namespaced cloudprofiles in namespace ${namespace}`) - - expect(authorization.canListNamespacedCloudProfiles).toHaveBeenCalledWith(user, namespace) + authorization.canListNamespacedCloudProfiles.mockResolvedValue(false) + + await expect( + namespacedCloudProfiles.listForNamespace({ user, namespace }), + ).rejects.toThrow( + expect.objectContaining({ + message: `You are not allowed to list namespaced cloudprofiles in namespace ${namespace}`, + }), + ) + + expect(authorization.canListNamespacedCloudProfiles).toHaveBeenCalledWith(user, namespace)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/__tests__/services.namespacedCloudProfiles.spec.js` around lines 236 - 251, The test calls namespacedCloudProfiles.listForNamespace twice but only stubs authorization.canListNamespacedCloudProfiles once; either stub twice or (preferred) invoke listForNamespace a single time and assert both the error class and message from that single invocation: call await expect(namespacedCloudProfiles.listForNamespace({ user, namespace })).rejects.toThrow(Forbidden) only once (or capture the thrown error via try/catch) and then assert the error.message contains `You are not allowed to list namespaced cloudprofiles in namespace ${namespace}`; ensure authorization.canListNamespacedCloudProfiles is still mocked with mockResolvedValueOnce(false) and keep the final expect(authorization.canListNamespacedCloudProfiles).toHaveBeenCalledWith(user, namespace).
195-208: Dynamic imports inbeforeEachre-import modules on every test — considerbeforeAllfor module loading.Because
vi.mockhoists to the top of the file, the mocks are already in place before anyimport()call. Re-importing in everybeforeEachadds overhead without changing behavior. If the intent is simply to get references to the mocked modules, a singlebeforeAllimport (plusvi.clearAllMocks()inafterEach) would suffice and be faster.This is a style preference and the current approach is functionally correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/__tests__/services.namespacedCloudProfiles.spec.js` around lines 195 - 208, Move the repeated dynamic imports out of beforeEach and into a single beforeAll to avoid re-importing mocked modules each test; specifically, import http-errors (assign Forbidden), ../lib/services/authorization.js (authorization), ../lib/cache/index.js (cache), and ../lib/services/namespacedCloudProfiles.js (namespacedCloudProfiles) inside beforeAll instead of beforeEach, and add vi.clearAllMocks() in afterEach to reset mock call state while preserving the existing test behavior.backend/lib/services/namespacedCloudProfiles.js (1)
24-40:computeDiffreturnsnullfor missing parent — downstream handles it, but silently.When the parent
CloudProfileis not found in the cache (line 28-30),computeDiffreturnsnull.transformToDiffthen setscloudProfileSpecDiff: null(line 55), which is the same sentinel used for "identical specs" (line 450 in the test). The frontend cannot distinguish between "parent missing" and "no diff from parent."Consider logging a warning when the parent is not found, so operators can detect stale or misconfigured
NamespacedCloudProfilereferences rather than silently treating them as unchanged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/services/namespacedCloudProfiles.js` around lines 24 - 40, computeDiff currently returns null when the parent CloudProfile is missing, which is ambiguous downstream; update computeDiff to log a warning before returning null so operators can detect missing parent references: inside computeDiff, after deriving parentName and before returning null when parentCloudProfile is falsy, emit a warning containing parentName and the namespacedCloudProfile identifier (e.g. namespacedCloudProfile.metadata.name/namespace or the whole object) using the module's logger or console.warn if no logger exists, then return null as before; keep the rest of computeDiff unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/jest.globalSetup.cjs`:
- Around line 14-20: The fallback in the catch block currently calls the
macOS-only `md5` binary via execSync; replace this with a portable Node-based
hash: run the same `find ... | sort` pipeline with execSync to get the file
list, then use Node's `crypto.createHash('md5')` and `fs.readFileSync`
(resolving paths with `path.resolve`) to accumulate file contents and return
`hash.digest('hex')` instead of shelling out to `md5`; update the execSync usage
in the catch block (the block that currently returns execSync(... 'md5'
...).trim()) to implement this Node-crypto fallback so it works on Linux CI.
In `@backend/lib/cache/index.js`:
- Around line 77-90: getNamespacedCloudProfiles is returning live references
from cache.getNamespacedCloudProfiles, and those objects are later mutated by
simplifyCloudProfile/simplifyObjectMetadata causing cache corruption; change
getNamespacedCloudProfiles to return deep-cloned copies (e.g., map each item
through _.cloneDeep) when returning items for a namespace or all items,
mirroring the cloneDeep behavior used in getCloudProfile(name).
In `@backend/lib/services/cloudprofiles.js`:
- Around line 19-22: The code calls simplifyCloudProfile directly on objects
returned by getCloudProfiles(), which mutates the informer cache; to fix, clone
each item before simplifying so the live cache isn't modified — e.g., in the
list path replace direct mapping with mapping that first deep-clones the object
(using structuredClone or a deep clone utility like lodash.cloneDeep) and then
passes the clone to simplifyCloudProfile; update references to
getCloudProfiles(), simplifyCloudProfile, and any callers of list() to ensure
only clones are mutated.
In `@backend/lib/services/namespacedCloudProfiles.js`:
- Around line 68-69: simplifyCloudProfile mutates its input (it calls
simplifyObjectMetadata which deletes metadata and uses _.set to modify
providerConfig), and you are passing cached objects returned by
getNamespacedCloudProfiles directly into items.map which corrupts the cache; fix
by deep-cloning cached entries before simplifying (e.g., clone each item
returned from getNamespacedCloudProfiles before calling simplifyCloudProfile) so
simplifyCloudProfile operates on a copy, and apply the same defensive clone
change in the equivalent cloudprofiles.js usage to avoid cache mutation there as
well.
In `@backend/lib/utils/index.js`:
- Around line 150-185: simplifyCloudProfile mutates its input (via
simplifyObjectMetadata and _.set), so update every call site that passes cached
objects to first clone them: change usages that call
simplifyCloudProfile(getCloudProfiles() mapped items) and
simplifyCloudProfile(getNamespacedCloudProfiles(...) mapped or filtered items)
to pass a deep clone (e.g., simplifyCloudProfile(_.cloneDeep(item))) — follow
the existing safe pattern used where
simplifyCloudProfile(_.cloneDeep(parentCloudProfile)) is already used; ensure
all three unsafe mappings use _.cloneDeep before calling simplifyCloudProfile.
- Around line 157-160: simplifyCloudProfile currently only preserves firewall
fields for type === 'ironcore' and 'openstack', so metal providerConfig entries
get stripped by stripProviderConfig; update simplifyCloudProfile (and the
allowlist logic) to treat type === 'metal' the same as 'ironcore' by using the
allowedPathsIroncore allowlist (or include 'metal' in the same conditional that
checks for 'ironcore') so firewallImages and firewallNetworks remain in
providerConfig for metal clouds; adjust any related comments/tests if present.
---
Nitpick comments:
In `@backend/__fixtures__/index.cjs`:
- Line 28: Inconsistent require path: change the require call that assigns
namespacedcloudprofiles (currently "require('./namespacedcloudprofiles.cjs')")
to match the other fixtures by removing the ".cjs" extension (i.e.,
require('./namespacedcloudprofiles')), or alternatively add the ".cjs" extension
to the other fixture requires for consistency; update the require expression
that references namespacedcloudprofiles to the chosen consistent form so imports
across the file follow the same pattern.
In `@backend/__fixtures__/namespacedcloudprofiles.cjs`:
- Line 11: Rename the confusing parameter name "kind" in the
getNamespacedCloudProfile function to a clearer name like "providerType" (or
"infraType") and update all internal references where it is used to set
status.cloudProfileSpec.type and to build machine type name prefixes; keep
spec.parent.kind hardcoded as 'CloudProfile'. Specifically, change the parameter
in getNamespacedCloudProfile and replace usages that assign to
status.cloudProfileSpec.type and any prefix generation logic to use
providerType, and update any call sites of getNamespacedCloudProfile to pass the
renamed argument.
In `@backend/__tests__/services.namespacedCloudProfiles.spec.js`:
- Around line 236-251: The test calls namespacedCloudProfiles.listForNamespace
twice but only stubs authorization.canListNamespacedCloudProfiles once; either
stub twice or (preferred) invoke listForNamespace a single time and assert both
the error class and message from that single invocation: call await
expect(namespacedCloudProfiles.listForNamespace({ user, namespace
})).rejects.toThrow(Forbidden) only once (or capture the thrown error via
try/catch) and then assert the error.message contains `You are not allowed to
list namespaced cloudprofiles in namespace ${namespace}`; ensure
authorization.canListNamespacedCloudProfiles is still mocked with
mockResolvedValueOnce(false) and keep the final
expect(authorization.canListNamespacedCloudProfiles).toHaveBeenCalledWith(user,
namespace).
- Around line 195-208: Move the repeated dynamic imports out of beforeEach and
into a single beforeAll to avoid re-importing mocked modules each test;
specifically, import http-errors (assign Forbidden),
../lib/services/authorization.js (authorization), ../lib/cache/index.js (cache),
and ../lib/services/namespacedCloudProfiles.js (namespacedCloudProfiles) inside
beforeAll instead of beforeEach, and add vi.clearAllMocks() in afterEach to
reset mock call state while preserving the existing test behavior.
In `@backend/jest.globalSetup.cjs`:
- Around line 37-47: The exported function globalSetup is declared async but
never uses await; remove the unnecessary async keyword to make it a synchronous
function instead (change "module.exports = async function globalSetup () {" to
"module.exports = function globalSetup () {"), keeping the existing calls to
shouldRebuildPackages() and execSync unchanged so behavior remains identical.
In `@backend/lib/services/namespacedCloudProfiles.js`:
- Around line 24-40: computeDiff currently returns null when the parent
CloudProfile is missing, which is ambiguous downstream; update computeDiff to
log a warning before returning null so operators can detect missing parent
references: inside computeDiff, after deriving parentName and before returning
null when parentCloudProfile is falsy, emit a warning containing parentName and
the namespacedCloudProfile identifier (e.g.
namespacedCloudProfile.metadata.name/namespace or the whole object) using the
module's logger or console.warn if no logger exists, then return null as before;
keep the rest of computeDiff unchanged.
In `@backend/rollup.config.js`:
- Line 28: sourcemap is always true in the Rollup config; change it to be
conditional on the build environment by introducing an isProduction flag (e.g.,
derived from process.env.NODE_ENV or a build-mode variable) and set the Rollup
output.sourcemap to false when isProduction is true and true otherwise; update
the rollup config where the sourcemap property is set (the output.sourcemap
entry) so production builds skip emitting .map files while dev builds keep them.
In `@frontend/__tests__/utils/index.spec.js`:
- Around line 552-559: Add explicit unit tests covering the case where status
exists but cloudProfileSpec is missing or null: in
frontend/__tests__/utils/index.spec.js add two tests calling getCloudProfileSpec
with a NamespacedCloudProfile that has metadata and spec but status: {} and
another with status: { cloudProfileSpec: null }, and assert both return {} to
verify the ?? fallback; reference the existing test for 'should return empty
object for NamespacedCloudProfile without status' and replicate its structure
but set status explicitly to {} and to { cloudProfileSpec: null } while keeping
the expect(...).toEqual({}).
In `@frontend/src/composables/useCloudProfile/useMachineTypes.js`:
- Around line 70-71: Replace the direct property access of regions on the spec
with a safe lodash get; instead of calling
find(getCloudProfileSpec(cloudProfile.value).regions, ...) use
find(get(getCloudProfileSpec(cloudProfile.value), ['regions'], []), { name:
region }) so regionObject and subsequently regionZones use a consistent safe
default; update the references to getCloudProfileSpec, regionObject,
regionZones, and find accordingly to match other files' pattern.
In `@frontend/src/composables/useShootHelper.js`:
- Around line 99-108: The inner const named namespace inside the computed
defaultCloudProfileRef shadows the outer computed namespace from
shootPropertyMappings; rename the inner variable (e.g., profileNamespace or
cpNamespace) and update its usage in the returned object (the kind check and
optional namespace spread) so defaultCloudProfileRef uses the new name while
leaving the outer namespace untouched; locate this change around the computed
defaultCloudProfileRef function and adjust the references to
get(defaultCloudProfile, ['metadata','namespace']) accordingly.
In `@frontend/src/store/cloudProfile/index.js`:
- Around line 138-156: The fallback in cloudProfileByRef that matches
namespacedList by name-only can return an ambiguous entry; update
cloudProfileByRef to emit a debug/warn log when the name-only fallback is used
so callers can trace ambiguous matches—specifically, inside cloudProfileByRef
when cloudProfileRef.kind === 'NamespacedCloudProfile' and
cloudProfileRef.namespace is falsy (the branch that calls
find(namespacedList.value, ['metadata.name', cloudProfileRef.name'])), add a
clear log statement (using the existing store logger or console.warn) that
includes cloudProfileRef.name and note that the namespace was missing and a
name-only match was returned, referencing namespacedList and the find call for
context.
In `@frontend/src/utils/index.js`:
- Around line 159-177: The optional chaining on cloudProfile in
getCloudProfileSpec is redundant after checking cloudProfile?.kind; update the
NamespacedCloudProfile branch to use cloudProfile.status?.cloudProfileSpec
(i.e., remove the leading ?. before status) so we still guard status but avoid
double-optional on cloudProfile; keep the fallback to {} unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
.yarn/cache/@dmsnell-diff-match-patch-npm-1.1.0-021308d314-8547bf4a62.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/jsondiffpatch-npm-0.7.3-5579555f56-fae49073e5.zipis excluded by!**/.yarn/**,!**/*.zipbackend/__tests__/acceptance/__snapshots__/api.allNamespacedCloudprofiles.spec.cjs.snapis excluded by!**/*.snapbackend/__tests__/acceptance/__snapshots__/api.namespacedCloudprofiles.spec.cjs.snapis excluded by!**/*.snapcharts/__tests__/gardener-dashboard/application/dashboard/__snapshots__/clusterrole.spec.js.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (47)
.pnp.cjsbackend/.gitignorebackend/__fixtures__/index.cjsbackend/__fixtures__/namespacedcloudprofiles.cjsbackend/__mocks__/jsondiffpatch.cjsbackend/__tests__/acceptance/api.allNamespacedCloudprofiles.spec.cjsbackend/__tests__/acceptance/api.cloudprofiles.spec.cjsbackend/__tests__/acceptance/api.namespacedCloudprofiles.spec.cjsbackend/__tests__/services.namespacedCloudProfiles.spec.jsbackend/jest.config.cjsbackend/jest.globalSetup.cjsbackend/jest.setup.cjsbackend/lib/cache/index.jsbackend/lib/hooks.jsbackend/lib/routes/allNamespacedCloudProfiles.jsbackend/lib/routes/index.jsbackend/lib/routes/namespacedCloudProfiles.jsbackend/lib/services/authorization.jsbackend/lib/services/cloudprofiles.jsbackend/lib/services/index.jsbackend/lib/services/namespacedCloudProfiles.jsbackend/lib/utils/index.jsbackend/package.jsonbackend/rollup.config.jsbackend/vitest.config.jsbackend/vitest.setup.jscharts/gardener-dashboard/charts/application/templates/dashboard/clusterrole.yamlfrontend/__tests__/stores/cloudProfile.spec.jsfrontend/__tests__/utils/index.spec.jsfrontend/package.jsonfrontend/src/components/GSelectCloudProfile.vuefrontend/src/composables/helper.jsfrontend/src/composables/useApi/api.jsfrontend/src/composables/useCloudProfile/useDefaultNodesCIDR.jsfrontend/src/composables/useCloudProfile/useKubernetesVersions.jsfrontend/src/composables/useCloudProfile/useMachineImages.jsfrontend/src/composables/useCloudProfile/useMachineTypes.jsfrontend/src/composables/useCloudProfile/useMetalConstraints.jsfrontend/src/composables/useCloudProfile/useOpenStackConstraints.jsfrontend/src/composables/useCloudProfile/useRegions.jsfrontend/src/composables/useCloudProfile/useVolumeTypes.jsfrontend/src/composables/useShootHelper.jsfrontend/src/router/guards.jsfrontend/src/store/cloudProfile/index.jsfrontend/src/store/seed.jsfrontend/src/utils/index.jspackages/kube-client/lib/resources/GardenerCore.js
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
backend/__tests__/services.namespacedCloudProfiles.spec.js (1)
289-289: Consider extracting repeated dynamic import to reduce duplication.
simplifyCloudProfileis dynamically imported in both the reconstruction test (line 289) and the kubernetes versions test (line 359). This could be moved tobeforeEachalongside other module imports to reduce duplication.Proposed refactor
let Forbidden + let simplifyCloudProfile beforeEach(async () => { // Import modules after mocks are set up const httpErrors = await import('http-errors') Forbidden = httpErrors.default.Forbidden const authModule = await import('../lib/services/authorization.js') authorization = authModule const cacheModule = await import('../lib/cache/index.js') cache = cacheModule.default const namespacedCloudProfilesModule = await import('../lib/services/namespacedCloudProfiles.js') namespacedCloudProfiles = namespacedCloudProfilesModule + + const utilsModule = await import('../lib/utils/index.js') + simplifyCloudProfile = utilsModule.simplifyCloudProfile })Then remove the inline imports at lines 289 and 359, and use
simplifyCloudProfiledirectly.Also applies to: 359-359
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/__tests__/services.namespacedCloudProfiles.spec.js` at line 289, The dynamic import of simplifyCloudProfile is duplicated in two tests; move that import into the test suite setup by importing simplifyCloudProfile in beforeEach (or a top-level beforeAll) alongside other module imports so tests can reference simplifyCloudProfile directly; remove the inline await import('../lib/utils/index.js') calls at the reconstruction test and the kubernetes versions test and use the simplifyCloudProfile variable provided by the setup instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/__tests__/services.namespacedCloudProfiles.spec.js`:
- Around line 236-251: The test is relying on implicit mock behavior by calling
namespacedCloudProfiles.listForNamespace twice while
authorization.canListNamespacedCloudProfiles has mockResolvedValueOnce(false)
only once; make the behavior explicit by either (A) setting
authorization.canListNamespacedCloudProfiles.mockResolvedValue(false) (or
mockResolvedValueOnce twice) so both calls get a falsy result, or (B) capture
the promise from namespacedCloudProfiles.listForNamespace({ user, namespace })
in a variable and run both rejection assertions against that same promise (so
you only invoke the function once); update assertions accordingly and keep
references to authorization.canListNamespacedCloudProfiles and
namespacedCloudProfiles.listForNamespace to locate the change.
In `@frontend/src/router/guards.js`:
- Around line 73-77: Normalize the route "namespace" param to a single string
before forwarding it to cloud-profile helpers: if (to.params.namespace ??
to.query.namespace) can be an array, detect Array.isArray(...) and pick the
first element (or coerce to string) so ensureCloudProfilesLoaded(namespace),
hasNamespacedCloudProfilesForNamespace(namespace) and
getNamespacedCloudProfiles(namespace) always receive a string; apply the same
normalization at the other occurrence around lines 219-225 to prevent array
values from breaking equality checks and encodeURIComponent usage.
In `@frontend/src/store/cloudProfile/index.js`:
- Around line 113-114: The infraProviderTypesList builder can include undefined
when getCloudProfileSpec(profile).type is missing; update the expression that
computes infraProviderTypesList (which uses allCloudProfiles,
getCloudProfileSpec, map, and uniq) to filter out empty/falsy provider types
before calling uniq—e.g., map to types then filter(Boolean) (or filter profiles
that have a valid spec.type first) so only non-empty provider type strings enter
the unique list used for UI vendor mapping.
- Around line 147-156: The current fallback in the NamespacedCloudProfile branch
matches only by metadata.name which can return the wrong profile if the same
name exists in multiple namespaces; instead, change the fallback so it either
(a) returns null when cloudProfileRef.namespace is missing to avoid ambiguous
resolution, or (b) filter namespacedList.value for items with metadata.name ===
cloudProfileRef.name and only return the item if exactly one match exists
(otherwise return null); update the logic around cloudProfileRef,
namespacedList, and the find/filter usage (and optionally add a brief debug/log
when multiple matches are found) to ensure no ambiguous profile is returned.
---
Nitpick comments:
In `@backend/__tests__/services.namespacedCloudProfiles.spec.js`:
- Line 289: The dynamic import of simplifyCloudProfile is duplicated in two
tests; move that import into the test suite setup by importing
simplifyCloudProfile in beforeEach (or a top-level beforeAll) alongside other
module imports so tests can reference simplifyCloudProfile directly; remove the
inline await import('../lib/utils/index.js') calls at the reconstruction test
and the kubernetes versions test and use the simplifyCloudProfile variable
provided by the setup instead.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/__tests__/services.namespacedCloudProfiles.spec.jsbackend/lib/routes/index.jsbackend/lib/services/namespacedCloudProfiles.jsfrontend/__tests__/stores/cloudProfile.spec.jsfrontend/__tests__/utils/index.spec.jsfrontend/src/composables/useApi/api.jsfrontend/src/router/guards.jsfrontend/src/store/cloudProfile/index.js
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/tests/stores/cloudProfile.spec.js
- frontend/src/composables/useApi/api.js
- backend/lib/routes/index.js
- backend/lib/services/namespacedCloudProfiles.js
frontend/src/router/guards.js
Outdated
Comment on lines
+73
to
+77
| const namespace = to.params.namespace ?? to.query.namespace | ||
| await Promise.all([ | ||
| ensureConfigLoaded(configStore), | ||
| ensureProjectsLoaded(projectStore), | ||
| ensureCloudProfilesLoaded(cloudProfileStore), | ||
| ensureCloudProfilesLoaded(cloudProfileStore, namespace), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
vue-router 4 route params query parameter types arrays repeated keys
💡 Result:
Web search failed: Server error: no LLM provider could handle the message
🏁 Script executed:
# Examine the guards.js file around the mentioned lines
cat -n frontend/src/router/guards.js | head -90 | tail -30Repository: klocke-io/dashboard
Length of output: 994
🏁 Script executed:
# Check lines 219-225 as well
cat -n frontend/src/router/guards.js | sed -n '215,230p'Repository: klocke-io/dashboard
Length of output: 645
🏁 Script executed:
# Find route definitions to understand namespace parameter structure
fd -e js -e ts | xargs rg -l "namespace.*path|path.*namespace" | head -10Repository: klocke-io/dashboard
Length of output: 125
🏁 Script executed:
# Search for route definitions in the router directory
find frontend/src/router -type f \( -name "*.js" -o -name "*.ts" \) -exec basename {} \;Repository: klocke-io/dashboard
Length of output: 114
🏁 Script executed:
# Check route definitions for namespace parameter
cat frontend/src/router/routes.js | head -100Repository: klocke-io/dashboard
Length of output: 2935
🏁 Script executed:
# Look for namespace param definitions
rg "namespace\s*[:=]" frontend/src/router/routes.js -A 2 -B 2Repository: klocke-io/dashboard
Length of output: 412
🏁 Script executed:
# Check cloudProfileStore methods
fd cloudProfile -t f | head -5Repository: klocke-io/dashboard
Length of output: 109
🏁 Script executed:
# Find where cloudProfileStore or similar stores are defined
find frontend/src -type f \( -name "*.js" -o -name "*.ts" \) | xargs rg "hasNamespacedCloudProfilesForNamespace|fetchNamespacedCloudProfiles" | head -20Repository: klocke-io/dashboard
Length of output: 647
🏁 Script executed:
# Check if this is Vue 2 or Vue 3, and vue-router version
cat package.json | rg "vue|router" -A 1Repository: klocke-io/dashboard
Length of output: 45
🏁 Script executed:
# Check package.json for vue and vue-router version
cat package.json | grep -A 5 '"vue"'Repository: klocke-io/dashboard
Length of output: 45
🏁 Script executed:
# Read cloudProfileStore implementation
cat frontend/src/store/cloudProfile/index.js | head -200Repository: klocke-io/dashboard
Length of output: 5609
🏁 Script executed:
# Look specifically at the two methods in question
rg "hasNamespacedCloudProfilesForNamespace|fetchNamespacedCloudProfiles" frontend/src/store/cloudProfile/index.js -A 10 -B 2Repository: klocke-io/dashboard
Length of output: 1149
🏁 Script executed:
# Check if there are any repeated query parameters used in routes
rg "query.*=" frontend/src/router/ -A 3 -B 3Repository: klocke-io/dashboard
Length of output: 487
🏁 Script executed:
# Check if namespace is ever constructed as an array or if there's defensive code elsewhere
rg "Array\.isArray|instanceof Array" frontend/src/router/ frontend/src/store/Repository: klocke-io/dashboard
Length of output: 418
🏁 Script executed:
# Look at how the API methods handle the namespace parameter
rg "getNamespacedCloudProfiles" frontend/src -B 5 -A 10 | head -50Repository: klocke-io/dashboard
Length of output: 3044
🏁 Script executed:
# Check if there are any tests or code paths that deliberately construct array namespaces
rg "namespace.*\[|Array.*namespace" frontend/ -t js --max-count=10Repository: klocke-io/dashboard
Length of output: 2375
🏁 Script executed:
# See if other guards handle query params with type checks
rg "to\.query\." frontend/src/router/guards.js -B 2 -A 2Repository: klocke-io/dashboard
Length of output: 349
Add type normalization for route namespace parameter to prevent array values from breaking API calls and cache checks.
At line 73, namespace is forwarded without type normalization. In vue-router 4, to.query.namespace may be an array if the query parameter is repeated (e.g., ?namespace=a&namespace=b). If an array is passed:
hasNamespacedCloudProfilesForNamespacefails due to strict equality check (array !== string)getNamespacedCloudProfilesreceives an array, causingencodeURIComponentto produce'a%2Cb'instead of the namespace string, resulting in malformed API paths
Suggested fix
- const namespace = to.params.namespace ?? to.query.namespace
+ const rawNamespace = to.params.namespace ?? to.query.namespace
+ const namespace = Array.isArray(rawNamespace) ? rawNamespace[0] : rawNamespace
await Promise.all([
ensureConfigLoaded(configStore),
ensureProjectsLoaded(projectStore),
ensureCloudProfilesLoaded(cloudProfileStore, namespace),Also applies to: 219-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/router/guards.js` around lines 73 - 77, Normalize the route
"namespace" param to a single string before forwarding it to cloud-profile
helpers: if (to.params.namespace ?? to.query.namespace) can be an array, detect
Array.isArray(...) and pick the first element (or coerce to string) so
ensureCloudProfilesLoaded(namespace),
hasNamespacedCloudProfilesForNamespace(namespace) and
getNamespacedCloudProfiles(namespace) always receive a string; apply the same
normalization at the other occurrence around lines 219-225 to prevent array
values from breaking equality checks and encodeURIComponent usage.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/__tests__/cache.spec.cjs (1)
34-34: Strengthen the defensive-copy assertion forgetCloudProfiles.At Line 34,
toStrictEqualon an empty array won’t catch aliasing. Since this PR targets cache-mutation safety, consider asserting non-reference equality with a non-empty list.Proposed test hardening
- const list = [] + const list = [{ metadata: { name: 'cp-1' } }] const stub = jest.spyOn(internalCache, 'getCloudProfiles').mockReturnValue(list) - expect(cache.getCloudProfiles()).toStrictEqual(list) + const result = cache.getCloudProfiles() + expect(result).toStrictEqual(list) + expect(result).not.toBe(list) expect(stub).toHaveBeenCalledTimes(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/__tests__/cache.spec.cjs` at line 34, The test currently uses toStrictEqual with an empty array which doesn't catch aliasing; update the assertion around getCloudProfiles to use a non-empty list and verify a defensive copy by asserting value equality plus non-reference identity — e.g., create a populated list, call cache.getCloudProfiles(), assert the returned array equals the original list content and also assert returnedArray !== originalList (use expect(...).not.toBe(...)), or alternatively mutate the returned array and assert the cached/original list remains unchanged; reference the getCloudProfiles function to locate the test change.frontend/src/store/cloudProfile/index.js (1)
140-142: Align JSDoc with implemented fallback behavior.The comment says “first match across all namespaces”, but the code returns only when the name is uniquely matched.
💡 Suggested doc update
- * by both name and namespace. If no namespace is provided in the ref, falls back to - * matching by name only (first match across all namespaces). + * by both name and namespace. If no namespace is provided in the ref, it returns a + * match only when the name is unique in the loaded namespaced list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/store/cloudProfile/index.js` around lines 140 - 142, The JSDoc currently says “first match across all namespaces” which is inaccurate; update the comment for the profile-matching function (the JSDoc above the code that inspects the ref object/name and namespace parameters) to state the actual fallback behavior: when ref.namespace is omitted the function will only return a profile if the profile name is unique across all namespaces (otherwise it does not resolve/returns nothing), or alternatively change the implementation to return the first match if you prefer that semantics—adjust the description of the ref/name/namespace handling to match the actual behavior exactly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/store/cloudProfile/index.js`:
- Around line 60-66: The function fetchNamespacedCloudProfiles currently returns
early when namespace is falsy, leaving the previous namespaced cache intact;
update fetchNamespacedCloudProfiles so that when namespace is missing it
explicitly clears the namespaced cache (e.g., call setNamespacedCloudProfiles
with an empty list or null for that namespace) to avoid stale entries in
allCloudProfiles, while preserving the normal behavior when namespace is
provided and you call api.getNamespacedCloudProfiles and
setNamespacedCloudProfiles(response.data, namespace).
---
Nitpick comments:
In `@backend/__tests__/cache.spec.cjs`:
- Line 34: The test currently uses toStrictEqual with an empty array which
doesn't catch aliasing; update the assertion around getCloudProfiles to use a
non-empty list and verify a defensive copy by asserting value equality plus
non-reference identity — e.g., create a populated list, call
cache.getCloudProfiles(), assert the returned array equals the original list
content and also assert returnedArray !== originalList (use
expect(...).not.toBe(...)), or alternatively mutate the returned array and
assert the cached/original list remains unchanged; reference the
getCloudProfiles function to locate the test change.
In `@frontend/src/store/cloudProfile/index.js`:
- Around line 140-142: The JSDoc currently says “first match across all
namespaces” which is inaccurate; update the comment for the profile-matching
function (the JSDoc above the code that inspects the ref object/name and
namespace parameters) to state the actual fallback behavior: when ref.namespace
is omitted the function will only return a profile if the profile name is unique
across all namespaces (otherwise it does not resolve/returns nothing), or
alternatively change the implementation to return the first match if you prefer
that semantics—adjust the description of the ref/name/namespace handling to
match the actual behavior exactly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/__tests__/cache.spec.cjsbackend/__tests__/services.namespacedCloudProfiles.spec.jsbackend/lib/cache/index.jsbackend/lib/services/namespacedCloudProfiles.jsbackend/lib/utils/index.jsfrontend/src/store/cloudProfile/index.js
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/tests/services.namespacedCloudProfiles.spec.js
- backend/lib/cache/index.js
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/src/store/cloudProfile/index.js (1)
60-66:⚠️ Potential issue | 🟠 MajorClear namespaced cache when namespace is absent.
At Line 61, early return preserves prior
namespacedList, so stale namespaced profiles can continue to appear throughallCloudProfiles.Proposed fix
async function fetchNamespacedCloudProfiles (namespace) { if (!namespace) { + setNamespacedCloudProfiles(null, null) return } const response = await api.getNamespacedCloudProfiles({ namespace }) setNamespacedCloudProfiles(response.data, namespace) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/store/cloudProfile/index.js` around lines 60 - 66, fetchNamespacedCloudProfiles currently returns early when namespace is falsy, leaving previous namespacedList cached and causing stale entries to surface via allCloudProfiles; modify fetchNamespacedCloudProfiles to clear the namespaced cache when namespace is absent by calling setNamespacedCloudProfiles with an empty list (or appropriate cleared value) for that namespace (or a global clear) before returning so the store no longer exposes stale profiles; reference fetchNamespacedCloudProfiles and setNamespacedCloudProfiles to locate where to insert the clear action.frontend/src/router/guards.js (1)
73-77:⚠️ Potential issue | 🟡 MinorNormalize
namespacebefore passing it to cloud-profile loaders.At Line 73,
namespaceis forwarded as-is. If it arrives as an array (e.g., repeated query keys), Line 223 cache checks and namespaced fetch behavior can break.Proposed fix
- const namespace = to.params.namespace ?? to.query.namespace + const rawNamespace = to.params.namespace ?? to.query.namespace + const namespace = Array.isArray(rawNamespace) ? rawNamespace[0] : rawNamespace await Promise.all([ ensureConfigLoaded(configStore), ensureProjectsLoaded(projectStore), ensureCloudProfilesLoaded(cloudProfileStore, namespace),- const namespace = to.params.namespace ?? to.query.namespace + const rawNamespace = to.params.namespace ?? to.query.namespace + const namespace = Array.isArray(rawNamespace) ? rawNamespace[0] : rawNamespace await refreshRules(authzStore, namespace)In Vue Router 4, what runtime type can `to.query.namespace` have when the URL includes repeated query params like `?namespace=a&namespace=b`?Also applies to: 219-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/router/guards.js` around lines 73 - 77, Normalize the namespace value before passing it to the cloud-profile loaders: compute a single-string namespace from to.params.namespace or to.query.namespace by checking Array.isArray(raw) and using raw[0] (or undefined) if it's an array, otherwise use the string; then pass that normalized variable into ensureCloudProfilesLoaded (and to the code paths around the cache checks / namespaced fetch logic referenced by the existing names like namespace in lines 219-225) so array-valued query params don't break caching or fetch behavior.
🧹 Nitpick comments (5)
frontend/src/composables/useCloudProfile/useDefaultNodesCIDR.js (1)
32-33: Consider updating the parameter doc to reflect expanded input support.Now that this reads through
getCloudProfileSpec, the JSDoc near Line 20 can be broadened beyondRef<CloudProfile>to include namespaced profiles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/composables/useCloudProfile/useDefaultNodesCIDR.js` around lines 32 - 33, Update the JSDoc in useDefaultNodesCIDR.js (near the existing param doc at Line 20) to reflect that the function now reads through getCloudProfileSpec and therefore accepts more than a Ref<CloudProfile>; explicitly document that the parameter may be a Ref or plain value of CloudProfile or a namespaced profile type (e.g., Ref<CloudProfile|NamespacedProfile> | CloudProfile | NamespacedProfile) and note that getCloudProfileSpec will be used to resolve the correct spec.frontend/src/composables/useCloudProfile/useMachineTypes.js (1)
60-64: Potential mutation of shared data.The code mutates
item.architecturedirectly on objects frommachineTypes.value. IfgetCloudProfileSpecreturns cached or shared data (which the PR commits mention - "cloning cloudprofiles in cache to prevent mutation"), this could inadvertently modify the cache.Consider creating shallow copies before mutation:
♻️ Proposed fix to avoid mutating source data
function filterMachineTypes (region, zones) { const machineAndVolumeTypePredicate = unavailableItems => { return item => { if (item.usable === false) { return false } if (includes(unavailableItems, item.name)) { return false } return true } } - const items = machineTypes.value - - items.forEach(item => { - if (!item.architecture) { - item.architecture = 'amd64' // default if not maintained - } - }) + const items = machineTypes.value.map(item => { + if (!item.architecture) { + return { ...item, architecture: 'amd64' } // default if not maintained + } + return item + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/composables/useCloudProfile/useMachineTypes.js` around lines 60 - 64, The loop in useMachineTypes.js mutates objects from items (from machineTypes.value / getCloudProfileSpec) by setting item.architecture, which can change cached/shared data; instead create shallow copies before applying the default (e.g., map items to new objects and set architecture only on the copy) so the original cached objects are never mutated; update the items.forEach mutation site to produce new objects (preserving other fields) and use those copies for downstream usage.charts/gardener-dashboard/charts/application/templates/dashboard/clusterrole.yaml (1)
53-59: RBAC permissions are appropriate; minor YAML formatting inconsistency.The new rule correctly grants
listandwatchpermissions fornamespacedcloudprofiles, following the principle of least privilege consistent with other resources in this ClusterRole.Note: The indentation style differs slightly from other rules (lines 54, 56 use 4-space indent for list items vs 2-space elsewhere). This is functionally correct but inconsistent.
📝 Optional: Align formatting with existing rules
- apiGroups: - - core.gardener.cloud + - core.gardener.cloud resources: - - namespacedcloudprofiles + - namespacedcloudprofiles verbs: - - list - - watch + - list + - watch🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/gardener-dashboard/charts/application/templates/dashboard/clusterrole.yaml` around lines 53 - 59, The YAML for the ClusterRole rule granting verbs for the resource namespacedcloudprofiles is functionally correct but uses inconsistent indentation for list items; update the rule block that contains apiGroups: core.gardener.cloud, resources: - namespacedcloudprofiles, and verbs: - list / - watch (the namespacedcloudprofiles rule) to match the project's existing two-space list-item indentation style so formatting is consistent with other ClusterRole rules.frontend/__tests__/stores/cloudProfile.spec.js (1)
102-109: Add an ambiguity regression test for namespaced refs without namespace.Line 102 currently validates only the unique-name fallback. Add a case with duplicate names across two namespaces and assert
nullto lock in the non-ambiguous contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/stores/cloudProfile.spec.js` around lines 102 - 109, Add a regression test to cover ambiguous NamespacedCloudProfile refs without namespace: set up two CloudProfile objects with the same metadata.name ('custom-gcp') in different metadata.namespaces, then call cloudProfileStore.cloudProfileByRef({ kind: 'NamespacedCloudProfile', name: 'custom-gcp' }) and assert the result is null to ensure ambiguity returns null (reference cloudProfileStore.cloudProfileByRef and the existing fallback test for guidance).frontend/src/store/cloudProfile/index.js (1)
140-142: Update JSDoc to match actual fallback behavior.Lines 140-142 describe “first match across all namespaces,” but Lines 157-160 now return only a unique match (otherwise
null).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/store/cloudProfile/index.js` around lines 140 - 142, Update the JSDoc above the cloud profile lookup function in frontend/src/store/cloudProfile/index.js to reflect the actual fallback behavior: it does not return the "first match across all namespaces" but instead returns a unique match or null when ambiguity exists; revise the description to state that if no namespace is provided the function searches across namespaces but will only return a profile when there is exactly one matching name (otherwise it returns null), referencing the lookup implementation that returns a unique match (the code block around the return handling currently at lines 157-160) so the docstring matches the behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/lib/cache/index.js`:
- Around line 77-82: getCloudProfile currently returns a direct reference into
the cache which downstream functions like simplifyCloudProfile (and its helpers
simplifyObjectMetadata and _.set) mutate, corrupting the cache; change
getCloudProfile to return a deep clone of the found profile (use _.cloneDeep on
the result of _.chain(cache.getCloudProfiles()).find(['metadata.name',
name]).value()) so callers get an isolated object (mirror the approach used in
getSeed).
---
Duplicate comments:
In `@frontend/src/router/guards.js`:
- Around line 73-77: Normalize the namespace value before passing it to the
cloud-profile loaders: compute a single-string namespace from
to.params.namespace or to.query.namespace by checking Array.isArray(raw) and
using raw[0] (or undefined) if it's an array, otherwise use the string; then
pass that normalized variable into ensureCloudProfilesLoaded (and to the code
paths around the cache checks / namespaced fetch logic referenced by the
existing names like namespace in lines 219-225) so array-valued query params
don't break caching or fetch behavior.
In `@frontend/src/store/cloudProfile/index.js`:
- Around line 60-66: fetchNamespacedCloudProfiles currently returns early when
namespace is falsy, leaving previous namespacedList cached and causing stale
entries to surface via allCloudProfiles; modify fetchNamespacedCloudProfiles to
clear the namespaced cache when namespace is absent by calling
setNamespacedCloudProfiles with an empty list (or appropriate cleared value) for
that namespace (or a global clear) before returning so the store no longer
exposes stale profiles; reference fetchNamespacedCloudProfiles and
setNamespacedCloudProfiles to locate where to insert the clear action.
---
Nitpick comments:
In
`@charts/gardener-dashboard/charts/application/templates/dashboard/clusterrole.yaml`:
- Around line 53-59: The YAML for the ClusterRole rule granting verbs for the
resource namespacedcloudprofiles is functionally correct but uses inconsistent
indentation for list items; update the rule block that contains apiGroups:
core.gardener.cloud, resources: - namespacedcloudprofiles, and verbs: - list / -
watch (the namespacedcloudprofiles rule) to match the project's existing
two-space list-item indentation style so formatting is consistent with other
ClusterRole rules.
In `@frontend/__tests__/stores/cloudProfile.spec.js`:
- Around line 102-109: Add a regression test to cover ambiguous
NamespacedCloudProfile refs without namespace: set up two CloudProfile objects
with the same metadata.name ('custom-gcp') in different metadata.namespaces,
then call cloudProfileStore.cloudProfileByRef({ kind: 'NamespacedCloudProfile',
name: 'custom-gcp' }) and assert the result is null to ensure ambiguity returns
null (reference cloudProfileStore.cloudProfileByRef and the existing fallback
test for guidance).
In `@frontend/src/composables/useCloudProfile/useDefaultNodesCIDR.js`:
- Around line 32-33: Update the JSDoc in useDefaultNodesCIDR.js (near the
existing param doc at Line 20) to reflect that the function now reads through
getCloudProfileSpec and therefore accepts more than a Ref<CloudProfile>;
explicitly document that the parameter may be a Ref or plain value of
CloudProfile or a namespaced profile type (e.g.,
Ref<CloudProfile|NamespacedProfile> | CloudProfile | NamespacedProfile) and note
that getCloudProfileSpec will be used to resolve the correct spec.
In `@frontend/src/composables/useCloudProfile/useMachineTypes.js`:
- Around line 60-64: The loop in useMachineTypes.js mutates objects from items
(from machineTypes.value / getCloudProfileSpec) by setting item.architecture,
which can change cached/shared data; instead create shallow copies before
applying the default (e.g., map items to new objects and set architecture only
on the copy) so the original cached objects are never mutated; update the
items.forEach mutation site to produce new objects (preserving other fields) and
use those copies for downstream usage.
In `@frontend/src/store/cloudProfile/index.js`:
- Around line 140-142: Update the JSDoc above the cloud profile lookup function
in frontend/src/store/cloudProfile/index.js to reflect the actual fallback
behavior: it does not return the "first match across all namespaces" but instead
returns a unique match or null when ambiguity exists; revise the description to
state that if no namespace is provided the function searches across namespaces
but will only return a profile when there is exactly one matching name
(otherwise it returns null), referencing the lookup implementation that returns
a unique match (the code block around the return handling currently at lines
157-160) so the docstring matches the behavior.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
.yarn/cache/@dmsnell-diff-match-patch-npm-1.1.0-021308d314-8547bf4a62.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/jsondiffpatch-npm-0.7.3-5579555f56-fae49073e5.zipis excluded by!**/.yarn/**,!**/*.zipbackend/__tests__/acceptance/__snapshots__/api.namespacedCloudprofiles.spec.cjs.snapis excluded by!**/*.snapcharts/__tests__/gardener-dashboard/application/dashboard/__snapshots__/clusterrole.spec.js.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (42)
.pnp.cjsbackend/__fixtures__/index.cjsbackend/__fixtures__/namespacedcloudprofiles.cjsbackend/__mocks__/jsondiffpatch.cjsbackend/__tests__/acceptance/api.cloudprofiles.spec.cjsbackend/__tests__/acceptance/api.namespacedCloudprofiles.spec.cjsbackend/__tests__/cache.spec.cjsbackend/__tests__/services.namespacedCloudProfiles.spec.jsbackend/jest.config.cjsbackend/jest.setup.cjsbackend/lib/cache/index.jsbackend/lib/hooks.jsbackend/lib/routes/index.jsbackend/lib/routes/namespacedCloudProfiles.jsbackend/lib/services/authorization.jsbackend/lib/services/cloudprofiles.jsbackend/lib/services/index.jsbackend/lib/services/namespacedCloudProfiles.jsbackend/lib/utils/index.jsbackend/package.jsonbackend/vitest.config.jscharts/gardener-dashboard/charts/application/templates/dashboard/clusterrole.yamlfrontend/__tests__/stores/cloudProfile.spec.jsfrontend/__tests__/utils/index.spec.jsfrontend/package.jsonfrontend/src/components/GSelectCloudProfile.vuefrontend/src/composables/helper.jsfrontend/src/composables/useApi/api.jsfrontend/src/composables/useCloudProfile/useDefaultNodesCIDR.jsfrontend/src/composables/useCloudProfile/useKubernetesVersions.jsfrontend/src/composables/useCloudProfile/useMachineImages.jsfrontend/src/composables/useCloudProfile/useMachineTypes.jsfrontend/src/composables/useCloudProfile/useMetalConstraints.jsfrontend/src/composables/useCloudProfile/useOpenStackConstraints.jsfrontend/src/composables/useCloudProfile/useRegions.jsfrontend/src/composables/useCloudProfile/useVolumeTypes.jsfrontend/src/composables/useShootHelper.jsfrontend/src/router/guards.jsfrontend/src/store/cloudProfile/index.jsfrontend/src/store/seed.jsfrontend/src/utils/index.jspackages/kube-client/lib/resources/GardenerCore.js
klocke-io
force-pushed
the
feature/namespaced-cloud-profile-support
branch
from
31178b0 to
3438dec
Compare
Owner
Author
|
https://github.com/coderabbitai full review |
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/router/guards.js (1)
221-230:⚠️ Potential issue | 🟡 MinorNormalize
namespacebefore cache/API checks.At Line 221,
namespaceis used as-is. With vue-router 4, route params/query values may be arrays, which can breakhasNamespacedCloudProfilesForNamespacematching and downstream request path construction.Suggested fix
async function ensureCloudProfilesLoaded (store, namespace, namespaces = []) { + const normalizedNamespace = Array.isArray(namespace) ? namespace[0] : namespace if (store.isInitial) { await store.fetchCloudProfiles() } - if (namespace === '_all') { + if (normalizedNamespace === '_all') { await store.fetchNamespacedCloudProfilesForNamespaces(namespaces) return } - if (namespace && !store.hasNamespacedCloudProfilesForNamespace(namespace)) { - await store.fetchNamespacedCloudProfiles(namespace) + if (normalizedNamespace && !store.hasNamespacedCloudProfilesForNamespace(normalizedNamespace)) { + await store.fetchNamespacedCloudProfiles(normalizedNamespace) } }In vue-router 4.3.0, what are the runtime/TypeScript shapes of `to.params.<key>` and `to.query.<key>`? Can either be arrays (e.g., repeated query keys), and what normalization is recommended before passing to API helpers?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/router/guards.js` around lines 221 - 230, Normalize the incoming namespace and namespaces values in ensureCloudProfilesLoaded before any cache/API checks: coerce namespace from possible string|string[]|undefined to a single string (e.g., if Array.isArray(namespace) use the first element, fallback to empty string or null) and similarly map/flatten the namespaces array so each entry is a string; then use that normalized variable for calls to hasNamespacedCloudProfilesForNamespace, fetchNamespacedCloudProfiles, and fetchNamespacedCloudProfilesForNamespaces to avoid mismatches when vue-router params/query are arrays.
🧹 Nitpick comments (2)
.pnp.cjs (1)
1205-1213: Consider enforcing immutable dependency-state checks in CI.A
yarn install --immutablestep helps catch accidental.pnp.cjsdrift early.Also applies to: 1687-1687, 1710-1710, 1743-1743, 1819-1819, 4805-4834, 4890-4896, 11102-11111, 15729-15729, 15770-15825
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.pnp.cjs around lines 1205 - 1213, Add a CI step that runs yarn install --immutable (or yarn install --immutable --check-cache) to fail the build when .pnp.cjs or lockfile drift occurs; update your CI pipeline job (e.g., the install/dependencies step) to run that command before caching or proceeding, so any changes to .pnp.cjs (and the listed package entries like "@dmsnell/diff-match-patch") are detected and the build fails until the repo is intentionally updated.frontend/src/store/cloudProfile/index.js (1)
81-106: Consider adding error handling for malformed diffs.The
jsondiffpatchPatchcall at line 101 could throw ifcloudProfileSpecDiffis malformed. While unlikely in production (server-generated), defensive error handling would improve robustness.💡 Optional: Add try-catch for patch application
if (cloudProfileSpecDiff === null) { // No changes from parent — spec is identical profile.status.cloudProfileSpec = cloneDeep(parent.spec) } else { // Apply delta on top of parent spec - profile.status.cloudProfileSpec = jsondiffpatchPatch(cloneDeep(parent.spec), cloudProfileSpecDiff) + try { + profile.status.cloudProfileSpec = jsondiffpatchPatch(cloneDeep(parent.spec), cloudProfileSpecDiff) + } catch (error) { + console.warn(`Failed to rehydrate profile ${profile.metadata.name}:`, error) + return profile + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/store/cloudProfile/index.js` around lines 81 - 106, Wrap the jsondiffpatchPatch call inside rehydrateNamespacedProfiles in a try-catch to guard against malformed diffs: when applying cloudProfileSpecDiff (in function rehydrateNamespacedProfiles, symbol jsondiffpatchPatch, variable cloudProfileSpecDiff, and target profile.status.cloudProfileSpec) catch any exceptions, log the error (including profile identifier like profile.metadata.name), fall back to setting profile.status.cloudProfileSpec = cloneDeep(parent.spec) or leave the parent spec unchanged, and still delete profile.status.cloudProfileSpecDiff so the profile is normalized even on error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/__tests__/composables/useCloudProfile/useMachineTypes.spec.js`:
- Around line 128-140: The test creates the computed from
useFilteredMachineTypes(region, architecture) but never evaluates it, so the
defaulting logic never runs; fix it by forcing the computed to be accessed after
creating it (call or read the returned computed's .value) so
useFilteredMachineTypes actually executes its defaulting code — specifically
update the test that invokes useFilteredMachineTypes(region, architecture) (and
the local refs region and architecture) to capture the returned computed (from
useFilteredMachineTypes) and read its value (or otherwise access it) before
asserting on cloudProfile.spec.machineTypes.
---
Duplicate comments:
In `@frontend/src/router/guards.js`:
- Around line 221-230: Normalize the incoming namespace and namespaces values in
ensureCloudProfilesLoaded before any cache/API checks: coerce namespace from
possible string|string[]|undefined to a single string (e.g., if
Array.isArray(namespace) use the first element, fallback to empty string or
null) and similarly map/flatten the namespaces array so each entry is a string;
then use that normalized variable for calls to
hasNamespacedCloudProfilesForNamespace, fetchNamespacedCloudProfiles, and
fetchNamespacedCloudProfilesForNamespaces to avoid mismatches when vue-router
params/query are arrays.
---
Nitpick comments:
In @.pnp.cjs:
- Around line 1205-1213: Add a CI step that runs yarn install --immutable (or
yarn install --immutable --check-cache) to fail the build when .pnp.cjs or
lockfile drift occurs; update your CI pipeline job (e.g., the
install/dependencies step) to run that command before caching or proceeding, so
any changes to .pnp.cjs (and the listed package entries like
"@dmsnell/diff-match-patch") are detected and the build fails until the repo is
intentionally updated.
In `@frontend/src/store/cloudProfile/index.js`:
- Around line 81-106: Wrap the jsondiffpatchPatch call inside
rehydrateNamespacedProfiles in a try-catch to guard against malformed diffs:
when applying cloudProfileSpecDiff (in function rehydrateNamespacedProfiles,
symbol jsondiffpatchPatch, variable cloudProfileSpecDiff, and target
profile.status.cloudProfileSpec) catch any exceptions, log the error (including
profile identifier like profile.metadata.name), fall back to setting
profile.status.cloudProfileSpec = cloneDeep(parent.spec) or leave the parent
spec unchanged, and still delete profile.status.cloudProfileSpecDiff so the
profile is normalized even on error.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
.yarn/cache/@dmsnell-diff-match-patch-npm-1.1.0-021308d314-8547bf4a62.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/jsondiffpatch-npm-0.7.3-5579555f56-fae49073e5.zipis excluded by!**/.yarn/**,!**/*.zipbackend/__tests__/acceptance/__snapshots__/api.namespacedCloudprofiles.spec.cjs.snapis excluded by!**/*.snapcharts/__tests__/gardener-dashboard/application/dashboard/__snapshots__/clusterrole.spec.js.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (44)
.pnp.cjsbackend/__fixtures__/index.cjsbackend/__fixtures__/namespacedcloudprofiles.cjsbackend/__mocks__/jsondiffpatch.cjsbackend/__tests__/acceptance/api.cloudprofiles.spec.cjsbackend/__tests__/acceptance/api.namespacedCloudprofiles.spec.cjsbackend/__tests__/cache.spec.cjsbackend/__tests__/services.namespacedCloudProfiles.spec.jsbackend/jest.config.cjsbackend/jest.setup.cjsbackend/lib/cache/index.jsbackend/lib/hooks.jsbackend/lib/routes/index.jsbackend/lib/routes/namespacedCloudProfiles.jsbackend/lib/services/authorization.jsbackend/lib/services/cloudprofiles.jsbackend/lib/services/index.jsbackend/lib/services/namespacedCloudProfiles.jsbackend/lib/utils/index.jsbackend/package.jsonbackend/vitest.config.jscharts/gardener-dashboard/charts/application/templates/dashboard/clusterrole.yamlfrontend/__tests__/composables/useCloudProfile/useMachineTypes.spec.jsfrontend/__tests__/stores/cloudProfile.spec.jsfrontend/__tests__/utils/index.spec.jsfrontend/package.jsonfrontend/src/components/GSelectCloudProfile.vuefrontend/src/composables/helper.jsfrontend/src/composables/useApi/api.jsfrontend/src/composables/useCloudProfile/useDefaultNodesCIDR.jsfrontend/src/composables/useCloudProfile/useKubernetesVersions.jsfrontend/src/composables/useCloudProfile/useMachineImages.jsfrontend/src/composables/useCloudProfile/useMachineTypes.jsfrontend/src/composables/useCloudProfile/useMetalConstraints.jsfrontend/src/composables/useCloudProfile/useOpenStackConstraints.jsfrontend/src/composables/useCloudProfile/useRegions.jsfrontend/src/composables/useCloudProfile/useVolumeTypes.jsfrontend/src/composables/useShootHelper.jsfrontend/src/composables/useShootMessages.jsfrontend/src/router/guards.jsfrontend/src/store/cloudProfile/index.jsfrontend/src/store/seed.jsfrontend/src/utils/index.jspackages/kube-client/lib/resources/GardenerCore.js
Comment on lines
+128
to
+140
| it('should not mutate cloud profile machine types when defaulting architecture', () => { | ||
| const { useZones } = useRegions(cloudProfile) | ||
| const { useFilteredMachineTypes } = useMachineTypes(cloudProfile, useZones) | ||
|
|
||
| const region = ref('region2') | ||
| const architecture = ref(undefined) | ||
| useFilteredMachineTypes(region, architecture) | ||
|
|
||
| const type2InProfile = find(cloudProfile.value.spec.machineTypes, { name: 'machineType2' }) | ||
| const type3InProfile = find(cloudProfile.value.spec.machineTypes, { name: 'machineType3' }) | ||
| expect(type2InProfile.architecture).toBeUndefined() | ||
| expect(type3InProfile.architecture).toBeUndefined() | ||
| }) |
There was a problem hiding this comment.
Force computed evaluation in the non-mutation test.
Line 134 currently creates the computed but never evaluates it, so the test can pass without running the defaulting logic.
💡 Proposed fix
const region = ref('region2')
const architecture = ref(undefined)
- useFilteredMachineTypes(region, architecture)
+ const dashboardMachineTypes = useFilteredMachineTypes(region, architecture)
+ // trigger computed execution
+ void dashboardMachineTypes.value
const type2InProfile = find(cloudProfile.value.spec.machineTypes, { name: 'machineType2' })
const type3InProfile = find(cloudProfile.value.spec.machineTypes, { name: 'machineType3' })
expect(type2InProfile.architecture).toBeUndefined()
expect(type3InProfile.architecture).toBeUndefined()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/__tests__/composables/useCloudProfile/useMachineTypes.spec.js`
around lines 128 - 140, The test creates the computed from
useFilteredMachineTypes(region, architecture) but never evaluates it, so the
defaulting logic never runs; fix it by forcing the computed to be accessed after
creating it (call or read the returned computed's .value) so
useFilteredMachineTypes actually executes its defaulting code — specifically
update the test that invokes useFilteredMachineTypes(region, architecture) (and
the local refs region and architecture) to capture the returned computed (from
useFilteredMachineTypes) and read its value (or otherwise access it) before
asserting on cloudProfile.spec.machineTypes.
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
1 participant
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.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note:
Summary by CodeRabbit
New Features
Bug Fixes
Chores