feat: Add support for namespacedcloudprofiles#2691
Open
klocke-io wants to merge 4 commits into
Open
Conversation
klocke-io
requested review from
grolu,
holgerkoser and
petersutter
as code owners
ghost
added
needs/review
klocke-io
force-pushed
the
feature/namespaced-cloud-profile-support
branch
3 times, most recently
from
a561dff to
674686f
Compare
|
@holgerkoser, @grolu You have pull request review open invite, please check |
grolu
added
area/ipcei
petersutter
reviewed
Dec 8, 2025
| '/projects': projectsRoute, | ||
| '/namespaces/:namespace/shoots': shootsRoute, | ||
| '/namespaces/:namespace/tickets': ticketsRoute, | ||
| '/namespaces/:namespace/cloudprofiles': namespacedCloudProfilesRoute, |
Member
There was a problem hiding this comment.
let's align closer to the gardener API (/apis/core.gardener.cloud/v1beta1/namespaces/garden-core/namespacedcloudprofiles)
Suggested change
| '/namespaces/:namespace/cloudprofiles': namespacedCloudProfilesRoute, | |
| '/namespaces/:namespace/namespacedcloudprofiles': namespacedCloudProfilesRoute, |
Contributor
|
The Gardener project currently lacks enough active contributors to adequately respond to all PRs.
You can:
/lifecycle stale |
ghost
added
the
lifecycle/stale
Contributor
|
The Gardener project currently lacks enough active contributors to adequately respond to all PRs.
You can:
/lifecycle rotten |
ghost
added
lifecycle/rotten
grolu
added
lifecycle/stale
Member
Author
|
/remove-lifecycle rotten |
klocke-io
removed
the
lifecycle/frozen
gardener-prow
Bot
added
the
do-not-merge/work-in-progress
klocke-io
force-pushed
the
feature/namespaced-cloud-profile-support
branch
from
674686f to
a3a62e7
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
gardener-prow
Bot
added
cla: yes
klocke-io
force-pushed
the
feature/namespaced-cloud-profile-support
branch
from
23fa0b5 to
6d52a64
Compare
📝 WalkthroughWalkthroughAdds support for NamespacedCloudProfile resources end-to-end: API, service, cache, informer, RBAC, fixtures, frontend store/composables/components, diff-based rehydration using jsondiffpatch, test coverage, and related tooling/config updates. NamespacedCloudProfile feature
sequenceDiagram
participant Client
participant FrontendStore
participant Server
participant Cache
participant Auth
participant jsondiffpatch
Client->>FrontendStore: request namespaced cloudprofiles (namespace, diff=true)
FrontendStore->>Server: GET /api/namespaces/:ns/namespacedcloudprofiles?diff=true
Server->>Auth: check canListNamespacedCloudProfiles(user, namespace)
Auth-->>Server: allowed/denied
Server->>Cache: getNamespacedCloudProfiles(namespace)
Cache-->>Server: namespaced profiles
Server->>Cache: getCloudProfile(parentName) [for each profile with diff]
Cache-->>Server: parent profile (or null)
Server->>jsondiffpatch: diff(parentSpec, childSpec) / patch(...) as needed
jsondiffpatch-->>Server: diff / patched spec
Server-->>Client: JSON response (diff-transformed or simplified profiles)
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
gardener-prow
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
frontend/src/composables/useShootMessages.js (1)
49-49: Prefer a more specific JSDoc shape thanobjectfor better DX.
Ref<object>works, but a minimal structural type would preserve IntelliSense while still covering both profile kinds.♻️ Optional JSDoc refinement
- * `@param` {Ref<object>} cloudProfile - A Vue ref containing a CloudProfile or NamespacedCloudProfile object + * `@param` {Ref<{ kind?: string, spec?: object }>} cloudProfile - A Vue ref containing a CloudProfile-like object🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/composables/useShootMessages.js` at line 49, The JSDoc for the cloudProfile parameter is too generic (Ref<object>) which hurts IntelliSense; update the JSDoc on the cloudProfile param in useShootMessages (the cloudProfile param of the function exported from frontend/src/composables/useShootMessages.js) to a minimal structural type that covers both CloudProfile and NamespacedCloudProfile (for example a Ref with metadata and optional namespace fields or an explicit union like Ref<CloudProfile|NamespacedCloudProfile>) so editors can infer properties like metadata.name/namespace while still accepting both kinds.backend/__tests__/acceptance/api.namespacedCloudprofiles.spec.cjs (1)
33-37: Add an explicit 200 assertion in the success path.This test already validates content type and body; adding
.expect(200)makes the contract stricter and clearer.✅ Suggested tweak
const res = await agent .get(`/api/namespaces/${namespace}/namespacedcloudprofiles`) .set('cookie', await user.cookie) + .expect(200) .expect('content-type', /json/)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/__tests__/acceptance/api.namespacedCloudprofiles.spec.cjs` around lines 33 - 37, The test's request chain lacks an explicit HTTP status assertion, so update the agent GET call to assert a 200 OK response by adding .expect(200) in the request chain (e.g., on the agent.get(`/api/namespaces/${namespace}/namespacedcloudprofiles`) call that currently sets 'cookie' and expects 'content-type' and inspects res.body); ensure .expect(200) is placed before or alongside the existing .expect('content-type', /json/) to make the success contract explicit.frontend/src/composables/useApi/api.js (1)
239-242: Consider makingdiffconfigurable in the API helper.Hardcoding
diff=truecouples all callers to diff computation. Exposing a parameter (default as needed) keeps this endpoint reusable and avoids unnecessary backend work.♻️ Proposed refactor
-export function getNamespacedCloudProfiles ({ namespace }) { +export function getNamespacedCloudProfiles ({ namespace, diff = true }) { namespace = encodeURIComponent(namespace) - return getResource(`/api/namespaces/${namespace}/namespacedcloudprofiles?diff=true`) + const search = new URLSearchParams({ diff: String(diff) }).toString() + return getResource(`/api/namespaces/${namespace}/namespacedcloudprofiles?${search}`) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/composables/useApi/api.js` around lines 239 - 242, Update getNamespacedCloudProfiles to accept a diff option instead of hardcoding diff=true: modify the function signature (getNamespacedCloudProfiles) to take an options object (e.g., { namespace, diff = true }) or an extra parameter, url-encode namespace as before, build the query string to include diff only when provided (using the diff value as default true if desired), and call getResource with `/api/namespaces/${namespace}/namespacedcloudprofiles?diff=${diff}` (or conditionally append the diff param) so callers can override or omit diff to avoid forcing diff computation.backend/lib/utils/index.js (1)
150-185: Consider makingsimplifyCloudProfilenon-mutating.At Line 168 and Line 182, the function mutates the passed object. Cloning internally would reduce side-effect risk when future callers pass shared references.
♻️ Suggested refactor
function simplifyCloudProfile (profile) { + profile = _.cloneDeep(profile) const kind = _.get(profile, ['kind']) let entrypath = ['spec'] if (kind === 'NamespacedCloudProfile') { entrypath = ['status', 'cloudProfileSpec']🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/utils/index.js` around lines 150 - 185, simplifyCloudProfile currently mutates its input by calling simplifyObjectMetadata(profile) and _.set(profile, [...entrypath, 'providerConfig'], strippedProviderConfig); to make it non-mutating, create a deep clone of the incoming profile at the start (e.g., cloneProfile = _.cloneDeep(profile)) and operate on that clone throughout (pass cloneProfile into simplifyObjectMetadata and set the providerConfig on cloneProfile instead of profile), and when calling stripProviderConfig ensure you also clone providerConfig before passing it if stripProviderConfig mutates its input; return the cloned/modified object (cloneProfile) and leave the original profile unchanged, referencing the simplifyCloudProfile function, profile variable, providerConfig variable, simplifyObjectMetadata and stripProviderConfig helpers, and the _.set usage to locate the changes.frontend/src/composables/useShootHelper.js (1)
102-107: Avoidnamespaceshadowing indefaultCloudProfileRef.The local
namespaceat Line 102 shadows the outernamespacecomputed ref, which makes this block harder to read.🧹 Small readability tweak
- 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 102 - 107, The local variable named "namespace" inside the object returned from defaultCloudProfile (const namespace = get(defaultCloudProfile, ['metadata', 'namespace'])) shadows the outer computed ref named "namespace" (defaultCloudProfileRef), hurting readability; rename the local to something like "profileNamespace" (or "cpNamespace") and update its usage in the returned object (kind: profileNamespace ? 'NamespacedCloudProfile' : 'CloudProfile', ...(profileNamespace && { namespace: profileNamespace })) so the outer computed ref remains unshadowed and the code is clearer.backend/__tests__/services.namespacedCloudProfiles.spec.js (1)
227-230: Prefer name-based assertions over index-based assertions in these tests.Using profile identity (e.g.,
metadata.name) instead of fixed positions will make tests less brittle to ordering changes.Also applies to: 269-272
🤖 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 227 - 230, Replace brittle index-based assertions on result[0]/result[1] with name-based assertions that check the set of profile names; for example, map result to an array of metadata.name and use something like expect(names).toEqual(expect.arrayContaining(['custom-cloudprofile-1','custom-cloudprofile-2'])) (or toContain for each name). Apply the same change to the other test block that asserts result[0]/result[1] (the second occurrence referenced) so both tests assert on metadata.name values rather than fixed positions.backend/lib/services/namespacedCloudProfiles.js (1)
24-39: Consider memoizing parent-spec simplification in diff mode.When multiple NamespacedCloudProfiles share a parent,
getCloudProfile+simplifyCloudProfileare recomputed for each item. Caching by parent name in the request scope would cut repeated work.Also applies to: 70-72
🤖 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 - 39, Multiple NamespacedCloudProfiles recompute getCloudProfile + simplifyCloudProfile for the same parent; add a request-scoped cache (e.g., a Map keyed by parentName) inside computeDiff to avoid repeated work: before calling getCloudProfile/simplifyCloudProfile check the cache for parentName, if present reuse the cached simplified parent or parentSpec, otherwise call getCloudProfile and simplifyCloudProfile, store the simplified result in the cache, then proceed to compute jsondiffpatch.diff; apply the same caching pattern to the other identical call site around lines 70-72 so both places reuse the request-scoped memoized simplified parent by parentName.
🤖 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`:
- Line 134: The test creates the computed from useFilteredMachineTypes(region,
architecture) but never evaluates it; change the test to capture the returned
computed (e.g., const filtered = useFilteredMachineTypes(region, architecture))
and then read or assign filtered.value so the computed callback runs and the
defaulting/mutation-sensitive logic executes; ensure any assertions use
filtered.value afterward to validate results.
In `@frontend/src/components/GSelectCloudProfile.vue`:
- Around line 38-49: The custom selection slot currently renders item.raw.title
directly which yields a blank label when the selected value isn't in
selectItems; update the selection template (the template `#selection`="{ item }"
slot) to use a safe fallback chain (e.g. item.raw.title || item.title ||
item.text || item.value) so a meaningful label is shown whenever item.raw.title
is empty or undefined and preserve the existing v-chip logic for
item.raw.namespaced.
In `@frontend/src/router/guards.js`:
- Line 73: The variable assigned to namespace (currently using
to.params.namespace ?? to.query.namespace) must be normalized to a single string
because to.query.namespace can be a string[]; update the assignment in guards.js
so that if to.query.namespace is an array you extract a single string (e.g.,
first element) or join/flatten it, and then fallback to to.params.namespace;
ensure the final namespace is a string (use Array.isArray on to.query.namespace
and coerce/trim as needed) before any validation or API calls that use
namespace.
In `@frontend/src/store/cloudProfile/index.js`:
- Around line 69-75: fetchNamespacedCloudProfilesForNamespaces currently only
adds missing namespaces but never removes stale entries, leaving
namespacedList/allCloudProfiles polluted; update this function to prune cache
entries for namespaces that are no longer in scope: compute the canonical set of
requested namespaces (uniq/filter(Boolean)), call the existing
fetchNamespacedCloudProfiles for any missing ones as before, and then remove any
keys from namespacedList and any entries in allCloudProfiles that belong to
namespaces not in that canonical set (use the store's mutation or helper that
manages namespacedList and allCloudProfiles so state stays consistent). Ensure
you reference and update namespacedList and allCloudProfiles within
fetchNamespacedCloudProfilesForNamespaces so removed namespaces are cleared
after the fetch step.
---
Nitpick comments:
In `@backend/__tests__/acceptance/api.namespacedCloudprofiles.spec.cjs`:
- Around line 33-37: The test's request chain lacks an explicit HTTP status
assertion, so update the agent GET call to assert a 200 OK response by adding
.expect(200) in the request chain (e.g., on the
agent.get(`/api/namespaces/${namespace}/namespacedcloudprofiles`) call that
currently sets 'cookie' and expects 'content-type' and inspects res.body);
ensure .expect(200) is placed before or alongside the existing
.expect('content-type', /json/) to make the success contract explicit.
In `@backend/__tests__/services.namespacedCloudProfiles.spec.js`:
- Around line 227-230: Replace brittle index-based assertions on
result[0]/result[1] with name-based assertions that check the set of profile
names; for example, map result to an array of metadata.name and use something
like
expect(names).toEqual(expect.arrayContaining(['custom-cloudprofile-1','custom-cloudprofile-2']))
(or toContain for each name). Apply the same change to the other test block that
asserts result[0]/result[1] (the second occurrence referenced) so both tests
assert on metadata.name values rather than fixed positions.
In `@backend/lib/services/namespacedCloudProfiles.js`:
- Around line 24-39: Multiple NamespacedCloudProfiles recompute getCloudProfile
+ simplifyCloudProfile for the same parent; add a request-scoped cache (e.g., a
Map keyed by parentName) inside computeDiff to avoid repeated work: before
calling getCloudProfile/simplifyCloudProfile check the cache for parentName, if
present reuse the cached simplified parent or parentSpec, otherwise call
getCloudProfile and simplifyCloudProfile, store the simplified result in the
cache, then proceed to compute jsondiffpatch.diff; apply the same caching
pattern to the other identical call site around lines 70-72 so both places reuse
the request-scoped memoized simplified parent by parentName.
In `@backend/lib/utils/index.js`:
- Around line 150-185: simplifyCloudProfile currently mutates its input by
calling simplifyObjectMetadata(profile) and _.set(profile, [...entrypath,
'providerConfig'], strippedProviderConfig); to make it non-mutating, create a
deep clone of the incoming profile at the start (e.g., cloneProfile =
_.cloneDeep(profile)) and operate on that clone throughout (pass cloneProfile
into simplifyObjectMetadata and set the providerConfig on cloneProfile instead
of profile), and when calling stripProviderConfig ensure you also clone
providerConfig before passing it if stripProviderConfig mutates its input;
return the cloned/modified object (cloneProfile) and leave the original profile
unchanged, referencing the simplifyCloudProfile function, profile variable,
providerConfig variable, simplifyObjectMetadata and stripProviderConfig helpers,
and the _.set usage to locate the changes.
In `@frontend/src/composables/useApi/api.js`:
- Around line 239-242: Update getNamespacedCloudProfiles to accept a diff option
instead of hardcoding diff=true: modify the function signature
(getNamespacedCloudProfiles) to take an options object (e.g., { namespace, diff
= true }) or an extra parameter, url-encode namespace as before, build the query
string to include diff only when provided (using the diff value as default true
if desired), and call getResource with
`/api/namespaces/${namespace}/namespacedcloudprofiles?diff=${diff}` (or
conditionally append the diff param) so callers can override or omit diff to
avoid forcing diff computation.
In `@frontend/src/composables/useShootHelper.js`:
- Around line 102-107: The local variable named "namespace" inside the object
returned from defaultCloudProfile (const namespace = get(defaultCloudProfile,
['metadata', 'namespace'])) shadows the outer computed ref named "namespace"
(defaultCloudProfileRef), hurting readability; rename the local to something
like "profileNamespace" (or "cpNamespace") and update its usage in the returned
object (kind: profileNamespace ? 'NamespacedCloudProfile' : 'CloudProfile',
...(profileNamespace && { namespace: profileNamespace })) so the outer computed
ref remains unshadowed and the code is clearer.
In `@frontend/src/composables/useShootMessages.js`:
- Line 49: The JSDoc for the cloudProfile parameter is too generic (Ref<object>)
which hurts IntelliSense; update the JSDoc on the cloudProfile param in
useShootMessages (the cloudProfile param of the function exported from
frontend/src/composables/useShootMessages.js) to a minimal structural type that
covers both CloudProfile and NamespacedCloudProfile (for example a Ref with
metadata and optional namespace fields or an explicit union like
Ref<CloudProfile|NamespacedCloudProfile>) so editors can infer properties like
metadata.name/namespace while still accepting both kinds.
ℹ️ 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
|
|
||
| const region = ref('region2') | ||
| const architecture = ref(undefined) | ||
| useFilteredMachineTypes(region, architecture) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the composable returns a lazy computed and the test doesn't dereference it.
rg -n -C2 "function useFilteredMachineTypes|return computed" frontend/src/composables/useCloudProfile/useMachineTypes.jsRepository: gardener/dashboard
Length of output: 661
🏁 Script executed:
#!/bin/bash
rg -n -C4 "should not mutate cloud profile machine types when defaulting architecture|useFilteredMachineTypes\(|\\.value" frontend/__tests__/composables/useCloudProfile/useMachineTypes.spec.jsRepository: gardener/dashboard
Length of output: 3281
Evaluate the computed value in this test to ensure the defaulting logic executes.
Line 134 creates the computed but never accesses it, so the mutation-sensitive logic inside the computed callback won't run. The test should assign and dereference .value to trigger the lazy-evaluated function.
🧪 Suggested fix
- useFilteredMachineTypes(region, architecture)
+ const dashboardMachineTypes = useFilteredMachineTypes(region, architecture)
+ void dashboardMachineTypes.value📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useFilteredMachineTypes(region, architecture) | |
| const dashboardMachineTypes = useFilteredMachineTypes(region, architecture) | |
| void dashboardMachineTypes.value |
🤖 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` at
line 134, The test creates the computed from useFilteredMachineTypes(region,
architecture) but never evaluates it; change the test to capture the returned
computed (e.g., const filtered = useFilteredMachineTypes(region, architecture))
and then read or assign filtered.value so the computed callback runs and the
defaulting/mutation-sensitive logic executes; ensure any assertions use
filtered.value afterward to validate results.
Comment on lines
+38
to
+49
| <template #selection="{ item }"> | ||
| <span>{{ item.raw.title }}</span> | ||
| <v-chip | ||
| v-if="item.raw.namespaced" | ||
| size="x-small" | ||
| color="primary" | ||
| variant="tonal" | ||
| class="ml-2" | ||
| > | ||
| Namespaced | ||
| </v-chip> | ||
| </template> |
There was a problem hiding this comment.
Selection label fallback is bypassed in the custom slot.
If the selected value is not present in selectItems, item.raw.title can be empty, so the field renders a blank selection label.
💡 Suggested fix
- <template `#selection`="{ item }">
- <span>{{ item.raw.title }}</span>
+ <template `#selection`="{ item }">
+ <span>{{ item.title }}</span>
<v-chip
- v-if="item.raw.namespaced"
+ v-if="item.raw.namespaced ?? item.raw.kind === 'NamespacedCloudProfile'"
size="x-small"
color="primary"
variant="tonal"
class="ml-2"
>
Namespaced
</v-chip>
</template>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/GSelectCloudProfile.vue` around lines 38 - 49, The
custom selection slot currently renders item.raw.title directly which yields a
blank label when the selected value isn't in selectItems; update the selection
template (the template `#selection`="{ item }" slot) to use a safe fallback chain
(e.g. item.raw.title || item.title || item.text || item.value) so a meaningful
label is shown whenever item.raw.title is empty or undefined and preserve the
existing v-chip logic for item.raw.namespaced.
| } | ||
|
|
||
| try { | ||
| const namespace = to.params.namespace ?? to.query.namespace |
There was a problem hiding this comment.
Normalize namespace to a single string before using it.
to.query.namespace may be string[]; forwarding it directly can cause invalid namespace checks and API calls.
💡 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🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/router/guards.js` at line 73, The variable assigned to namespace
(currently using to.params.namespace ?? to.query.namespace) must be normalized
to a single string because to.query.namespace can be a string[]; update the
assignment in guards.js so that if to.query.namespace is an array you extract a
single string (e.g., first element) or join/flatten it, and then fallback to
to.params.namespace; ensure the final namespace is a string (use Array.isArray
on to.query.namespace and coerce/trim as needed) before any validation or API
calls that use namespace.
Comment on lines
+69
to
+75
| async function fetchNamespacedCloudProfilesForNamespaces (namespaces = []) { | ||
| const namespacesToFetch = uniq(namespaces) | ||
| .filter(Boolean) | ||
| .filter(namespace => !hasNamespacedCloudProfilesForNamespace(namespace)) | ||
|
|
||
| await Promise.all(namespacesToFetch.map(namespace => fetchNamespacedCloudProfiles(namespace))) | ||
| } |
There was a problem hiding this comment.
Prune stale namespace cache entries when namespace scope shrinks.
Current logic only fetches missing namespaces; it never removes namespaces that are no longer in scope. That leaves stale namespaced cloud profiles in namespacedList / allCloudProfiles.
💡 Suggested fix
async function fetchNamespacedCloudProfilesForNamespaces (namespaces = []) {
- const namespacesToFetch = uniq(namespaces)
+ const allowedNamespaces = uniq(namespaces)
.filter(Boolean)
- .filter(namespace => !hasNamespacedCloudProfilesForNamespace(namespace))
+
+ // Drop cached namespaces that are no longer in scope
+ namespacedListsByNamespace.value = Object.fromEntries(
+ Object.entries(namespacedListsByNamespace.value)
+ .filter(([namespace]) => allowedNamespaces.includes(namespace)),
+ )
+ namespacedList.value = flatten(Object.values(namespacedListsByNamespace.value))
+
+ const namespacesToFetch = allowedNamespaces
+ .filter(namespace => !hasNamespacedCloudProfilesForNamespace(namespace))
await Promise.all(namespacesToFetch.map(namespace => fetchNamespacedCloudProfiles(namespace)))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function fetchNamespacedCloudProfilesForNamespaces (namespaces = []) { | |
| const namespacesToFetch = uniq(namespaces) | |
| .filter(Boolean) | |
| .filter(namespace => !hasNamespacedCloudProfilesForNamespace(namespace)) | |
| await Promise.all(namespacesToFetch.map(namespace => fetchNamespacedCloudProfiles(namespace))) | |
| } | |
| async function fetchNamespacedCloudProfilesForNamespaces (namespaces = []) { | |
| const allowedNamespaces = uniq(namespaces) | |
| .filter(Boolean) | |
| // Drop cached namespaces that are no longer in scope | |
| namespacedListsByNamespace.value = Object.fromEntries( | |
| Object.entries(namespacedListsByNamespace.value) | |
| .filter(([namespace]) => allowedNamespaces.includes(namespace)), | |
| ) | |
| namespacedList.value = flatten(Object.values(namespacedListsByNamespace.value)) | |
| const namespacesToFetch = allowedNamespaces | |
| .filter(namespace => !hasNamespacedCloudProfilesForNamespace(namespace)) | |
| await Promise.all(namespacesToFetch.map(namespace => fetchNamespacedCloudProfiles(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 69 - 75,
fetchNamespacedCloudProfilesForNamespaces currently only adds missing namespaces
but never removes stale entries, leaving namespacedList/allCloudProfiles
polluted; update this function to prune cache entries for namespaces that are no
longer in scope: compute the canonical set of requested namespaces
(uniq/filter(Boolean)), call the existing fetchNamespacedCloudProfiles for any
missing ones as before, and then remove any keys from namespacedList and any
entries in allCloudProfiles that belong to namespaces not in that canonical set
(use the store's mutation or helper that manages namespacedList and
allCloudProfiles so state stays consistent). Ensure you reference and update
namespacedList and allCloudProfiles within
fetchNamespacedCloudProfilesForNamespaces so removed namespaces are cleared
after the fetch step.
grolu
reviewed
Mar 9, 2026
Member
There was a problem hiding this comment.
use camel case for file like you did with the other files
Member
Author
There was a problem hiding this comment.
done, thanks for catching it :)
grolu
reviewed
Mar 9, 2026
| @@ -50,6 +50,13 @@ rules: | |||
| verbs: | |||
Member
There was a problem hiding this comment.
we also need to add this to gardener operator
Member
Author
There was a problem hiding this comment.
https://github.com/gardener/gardener/pull/13500/changes
Done and merged, or did you mean something else :)
petersutter
reviewed
Mar 10, 2026
| const parentCloudProfile = getCloudProfile(parentName) | ||
|
|
||
| if (!parentCloudProfile) { | ||
| return null |
Member
There was a problem hiding this comment.
Should this case throw an exception?
Comment on lines
+51
to
+58
| const result = _.cloneDeep(profile) | ||
| result.status = { | ||
| ...result.status, | ||
| cloudProfileSpecDiff: diff || null, | ||
| } | ||
| delete result.status.cloudProfileSpec | ||
|
|
||
| return result |
Member
There was a problem hiding this comment.
Suggested change
| const result = _.cloneDeep(profile) | |
| result.status = { | |
| ...result.status, | |
| cloudProfileSpecDiff: diff || null, | |
| } | |
| delete result.status.cloudProfileSpec | |
| return result | |
| const { status = {}, ...profileRest } = profile | |
| const { cloudProfileSpec, ...statusRest } = status | |
| return { | |
| ...profileRest, | |
| status: { | |
| ...statusRest, | |
| cloudProfileSpecDiff: diff ?? null, | |
| }, | |
| } |
| "test": "NODE_PATH=./dist cross-env NODE_ENV=test node --experimental-vm-modules $(yarn bin jest)", | ||
| "test-coverage": "yarn test --coverage" | ||
| "test-coverage": "yarn test --coverage", | ||
| "test-vitest": "vitest run" |
Member
There was a problem hiding this comment.
test-vitest is currently not run viamake verify, or?
| const parentName = profile.spec?.parent?.name | ||
| const parent = find(list.value, ['metadata.name', parentName]) | ||
| if (!parent) { | ||
| return profile |
Member
There was a problem hiding this comment.
isn't this an exception / should be handled as error case?
| ) ?? null | ||
| } | ||
| // Fallback: only return when name is unique across namespaces | ||
| const matches = filter(namespacedList.value, ['metadata.name', cloudProfileRef.name]) |
Member
There was a problem hiding this comment.
when namespace is not set on cloudProfileRef, the namespace of the parent object (e.g. the shoot) has to be taken
Comment on lines
+44
to
+46
| const isNamespacedInitial = computed(() => { | ||
| return Object.keys(namespacedListsByNamespace.value).length === 0 | ||
| }) |
|
PR needs rebase. DetailsInstructions 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. |
gardener-prow
Bot
added
the
needs-rebase
Contributor
|
The Gardener project currently lacks enough active contributors to adequately respond to all PRs.
You can:
/lifecycle stale |
gardener-prow
Bot
added
the
lifecycle/stale
…tion Backend returns parentResourceVersion and cloudProfileSpecHash alongside diffs. Frontend verifies rehydrated specs against the hash, re-fetches drifted parents, and drops profiles that cannot be reconciled — surfacing errors to the user via the app store.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/services/namespacedCloudProfiles.js`:
- Line 44: The backend is producing spec hashes using a non-canonical JSON
serialization (backend/lib/utils/index.js#computeSpecHash) which mismatches the
frontend's canonicalized hash (frontend/src/utils/crypto.js#computeSpecHash) and
causes false profile drops; update the code that sets cloudProfileSpecHash (in
namespacedCloudProfiles.js) to canonicalize the spec before hashing—either
replace the current compute call with the same canonicalization routine used by
the frontend or add a stable/canonical JSON serialize step (e.g., stableSort
keys or use the shared canonicalize function) so computeSpecHash receives a
canonical string and both sides produce identical hashes.
- Around line 38-45: parentSpec is taken from simplifiedParent but
namespacedSpec is pulled raw from namespacedCloudProfile, causing inconsistent
inputs to jsondiffpatch.diff and computeSpecHash; fix by first producing a
simplified version of namespacedCloudProfile the same way simplifiedParent was
produced (so get the ['spec'] from that simplified object) and then use that
simplified namespaced spec in both jsondiffpatch.diff(parentSpec,
namespacedSpec) and computeSpecHash(namespacedSpec) so both diff and hash
operate on the same simplified representation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f03736d-2863-4505-b7bf-525133c19249
⛔ Files ignored due to path filters (3)
.yarn/cache/canonicalize-npm-2.1.0-8ea3fe50f0-3b1ec61276.zipis excluded by!**/.yarn/**,!**/*.zipbackend/__tests__/acceptance/__snapshots__/api.cloudprofiles.spec.cjs.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
.pnp.cjsbackend/__fixtures__/cloudprofiles.cjsbackend/__tests__/acceptance/api.cloudprofiles.spec.cjsbackend/__tests__/services.namespacedCloudProfiles.spec.jsbackend/lib/routes/cloudprofiles.jsbackend/lib/services/cloudprofiles.jsbackend/lib/services/namespacedCloudProfiles.jsbackend/lib/utils/index.jsbackend/package.jsonfrontend/__tests__/stores/cloudProfile.spec.jsfrontend/package.jsonfrontend/src/composables/useApi/api.jsfrontend/src/store/cloudProfile/index.jsfrontend/src/utils/crypto.js
✅ Files skipped from review due to trivial changes (3)
- backend/fixtures/cloudprofiles.cjs
- backend/tests/acceptance/api.cloudprofiles.spec.cjs
- frontend/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/lib/utils/index.js
- frontend/src/composables/useApi/api.js
- backend/package.json
Comment on lines
+38
to
+45
| const parentSpec = _.get(simplifiedParent, ['spec'], {}) | ||
| const namespacedSpec = _.get(namespacedCloudProfile, ['status', 'cloudProfileSpec'], {}) | ||
|
|
||
| return { | ||
| diff: jsondiffpatch.diff(parentSpec, namespacedSpec), | ||
| parentResourceVersion, | ||
| cloudProfileSpecHash: computeSpecHash(namespacedSpec), | ||
| } |
There was a problem hiding this comment.
Diff/hash input is inconsistent with the simplified parent spec.
On Line 38 the parent spec is simplified, but on Line 39 the namespaced spec is taken raw. That makes cloudProfileSpecDiff and cloudProfileSpecHash depend on unsimplified fields (notably providerConfig), which can create avoidable drift/noise.
💡 Suggested fix
- const parentSpec = _.get(simplifiedParent, ['spec'], {})
- const namespacedSpec = _.get(namespacedCloudProfile, ['status', 'cloudProfileSpec'], {})
+ const parentSpec = _.get(simplifiedParent, ['spec'], {})
+ const rawNamespacedSpec = _.get(namespacedCloudProfile, ['status', 'cloudProfileSpec'], {})
+ const namespacedSpec = _.get(
+ simplifyCloudProfile({ spec: rawNamespacedSpec }),
+ ['spec'],
+ {},
+ )🤖 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 38 - 45,
parentSpec is taken from simplifiedParent but namespacedSpec is pulled raw from
namespacedCloudProfile, causing inconsistent inputs to jsondiffpatch.diff and
computeSpecHash; fix by first producing a simplified version of
namespacedCloudProfile the same way simplifiedParent was produced (so get the
['spec'] from that simplified object) and then use that simplified namespaced
spec in both jsondiffpatch.diff(parentSpec, namespacedSpec) and
computeSpecHash(namespacedSpec) so both diff and hash operate on the same
simplified representation.
| return { | ||
| diff: jsondiffpatch.diff(parentSpec, namespacedSpec), | ||
| parentResourceVersion, | ||
| cloudProfileSpecHash: computeSpecHash(namespacedSpec), |
There was a problem hiding this comment.
Backend/frontend spec-hash canonicalization is currently mismatched.
Line 44 relies on backend/lib/utils/index.js#computeSpecHash (JSON serialization order-sensitive), while the frontend rehydration path uses frontend/src/utils/crypto.js#computeSpecHash with canonicalization. Equal specs can hash differently and trigger false profile drops.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/lib/services/namespacedCloudProfiles.js` at line 44, The backend is
producing spec hashes using a non-canonical JSON serialization
(backend/lib/utils/index.js#computeSpecHash) which mismatches the frontend's
canonicalized hash (frontend/src/utils/crypto.js#computeSpecHash) and causes
false profile drops; update the code that sets cloudProfileSpecHash (in
namespacedCloudProfiles.js) to canonicalize the spec before hashing—either
replace the current compute call with the same canonicalization routine used by
the frontend or add a stable/canonical JSON serialize step (e.g., stableSort
keys or use the shared canonicalize function) so computeSpecHash receives a
canonical string and both sides produce identical hashes.
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
4 participants
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:
Add support for namespacedcloudprofiles
Which issue(s) this PR fixes:
Fixes # #1971
Special notes for your reviewer:
NSCP in create cluster dialog
No more warnings because the cloudprofile is not understood
Release note:
Summary by CodeRabbit
New Features
Tests