feat: link to managed seed shoot and combine readiness#2834
Open
petersutter wants to merge 3 commits intomasterfrom
Open
feat: link to managed seed shoot and combine readiness#2834petersutter wants to merge 3 commits intomasterfrom
petersutter wants to merge 3 commits intomasterfrom
Conversation
petersutter
requested review from
grolu,
holgerkoser and
klocke-io
as code owners
gardener-prow
bot
added
do-not-merge/needs-kind
📝 WalkthroughWalkthroughAdd end-to-end support for garden-scoped ManagedSeed and ManagedSeed-Shoot resources: cache accessors, services, routes, IO synchronize handlers and watches, authorization checks, RBAC, frontend stores/composables/components, router guard loading, socket wiring, and accompanying tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client
participant Router as RouterGuard
participant API as BackendAPI
participant Auth as AuthService
participant Cache as CacheStore
participant Socket as Socket.IO
Browser->>Router: navigate (seeds page, admin)
Router->>API: GET /api/namespaces/garden/managedseeds
API->>Auth: canListManagedSeedsInGardenNamespace(user)
Auth-->>API: allowed
API->>Cache: getManagedSeedsInGardenNamespace()
Cache-->>API: [simplified managedSeed objects]
API-->>Browser: 200 [simplified managed seeds]
Note over Socket,Cache: realtime sync for garden-scoped resources
Socket->>Cache: watch managedseeds / shoots (garden)
Cache-->>Socket: ADDED/MODIFIED/DELETED events
Socket-->>Browser: emit "managedseeds" / "managedseed-shoots" events
Browser->>Browser: update managedSeedStore / UI
Browser->>API: GET /api/namespaces/garden/managedseed-shoots
API->>Auth: canListManagedSeedsInGardenNamespace && canListShootsInGardenNamespace
Auth-->>API: allowed
API->>Cache: getManagedSeedsInGardenNamespace() / getShootByName(...)
Cache-->>API: shoot + status
API-->>Browser: 200 [simplified managed-seed-shoots]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
|
[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 |
petersutter
added
kind/enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (7)
frontend/__tests__/composables/useApi.spec.js (1)
36-52: Test looks good, but coverage forgetManagedSeedsForGardenNamespaceis missing.The test correctly verifies the
getManagedSeedShootsForGardenNamespaceendpoint. Consider adding a similar test forgetManagedSeedsForGardenNamespaceto maintain consistent coverage for both new API functions.💡 Suggested test for getManagedSeedsForGardenNamespace
describe('#getManagedSeedsForGardenNamespace', () => { it('should fetch managed seeds from the garden namespace endpoint', async () => { fetch.mockResponseOnce(JSON.stringify([]), { headers: { 'content-type': 'application/json; charset=UTF-8', }, }) const res = await api.getManagedSeedsForGardenNamespace() expect(res.status).toBe(200) expect(res.data).toEqual([]) expect(fetch).toBeCalledTimes(1) const [req] = fetch.mock.calls[0] expect(req.url).toBe('http://localhost:3000/api/namespaces/garden/managedseeds') }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/composables/useApi.spec.js` around lines 36 - 52, Add a new test block mirroring the existing describe('#getManagedSeedShootsForGardenNamespace') case to cover getManagedSeedsForGardenNamespace: mock fetch response with fetch.mockResponseOnce(JSON.stringify([]), { headers: { 'content-type': 'application/json; charset=UTF-8' } }), call api.getManagedSeedsForGardenNamespace(), assert res.status === 200 and res.data equals [], assert fetch called once, and assert the requested URL equals 'http://localhost:3000/api/namespaces/garden/managedseeds' so the new function getManagedSeedsForGardenNamespace is covered.frontend/src/store/managedSeedShoot.js (1)
50-52: Consider null-safety forshootByNamewhen list is uninitialized.The
findfunction from lodash will returnundefinedwhen called withnullas the collection, which is safe. However, if this behavior is intentional, it might be worth adding a brief comment or early return for clarity, especially sinceisInitialdistinguishes the null state.♻️ Optional: Add explicit null check for clarity
function shootByName (namespace, name) { + if (list.value === null) { + return undefined + } return find(list.value, { metadata: { namespace, name } }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/store/managedSeedShoot.js` around lines 50 - 52, The helper shootByName currently calls find(list.value, { metadata: { namespace, name } }) which can receive null when list is uninitialized; add an explicit null-safety guard in the shootByName function (check list.value or isInitial) and return null/undefined early if the list is not ready so callers see the intent clearly; reference the shootByName function and the list.value access and use an early return to avoid relying on implicit lodash behavior.backend/lib/watches/managedseeds.js (1)
22-24: Consider consistency in delete handler invocation.The
deletehandler passes the deleted object asnewObject:informer.on('delete', object => handleEvent('DELETED', object))While this works due to the
newObject ?? oldObjectfallback on line 13, semantically the deleted object is the "old" object. For consistency with theupdatehandler pattern and clearer intent:♻️ Optional: Pass deleted object as oldObject
informer.on('add', object => handleEvent('ADDED', object)) informer.on('update', (object, oldObject) => handleEvent('MODIFIED', object, oldObject)) -informer.on('delete', object => handleEvent('DELETED', object)) +informer.on('delete', object => handleEvent('DELETED', undefined, object))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/watches/managedseeds.js` around lines 22 - 24, The delete handler currently calls handleEvent('DELETED', object) which treats the deleted resource as the "new" object; change it to pass the deleted resource as the oldObject (i.e., call handleEvent('DELETED', undefined, object)) so it matches the update pattern and the function signature of handleEvent(newObject, oldObject).backend/__tests__/services.managedseedshoots.spec.cjs (1)
39-50: Add the inverse authorization case.This only proves the
canListManagedSeedsInGardenNamespace = falsepath. A regression wherecanListShootsInGardenNamespacestops being enforced would still pass this suite.💡 Suggested test
it('should reject unauthorized users', async () => { authorization.canListManagedSeedsInGardenNamespace.mockResolvedValue(false) await expect(managedseedshoots.list({ user, namespace: 'garden' })).rejects.toEqual(expect.objectContaining({ statusCode: 403, message: 'You are not allowed to list managed seed shoots in the garden namespace', })) expect(authorization.canListManagedSeedsInGardenNamespace).toHaveBeenCalledWith(user) expect(authorization.canListShootsInGardenNamespace).toHaveBeenCalledWith(user) expect(cache.getManagedSeedsInGardenNamespace).not.toHaveBeenCalled() }) + + it('should reject users without shoot access', async () => { + authorization.canListShootsInGardenNamespace.mockResolvedValue(false) + + await expect(managedseedshoots.list({ user, namespace: 'garden' })).rejects.toEqual(expect.objectContaining({ + statusCode: 403, + message: 'You are not allowed to list managed seed shoots in the garden namespace', + })) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/__tests__/services.managedseedshoots.spec.cjs` around lines 39 - 50, Add a test that covers the inverse authorization path by simulating canListManagedSeedsInGardenNamespace returning true but canListShootsInGardenNamespace returning false, then call managedseedshoots.list({ user, namespace: 'garden' }) and assert it rejects with a 403 and the same message; also assert that authorization.canListManagedSeedsInGardenNamespace was calledWith(user), authorization.canListShootsInGardenNamespace was calledWith(user), and that cache.getManagedSeedsInGardenNamespace was not called to ensure both checks are enforced.frontend/src/views/GSeedList.vue (1)
142-150: Sanitize persisted sort keys when the seed headers change.This PR changes the available columns, but the view still restores
seedSortByfrom local storage verbatim on mount. A stale key from an older session can leave the table in an invalid sort state until the user manually resets it.💡 Suggested follow-up
onMounted(() => { updateTableSettings() if (seedSortBy.value?.length) { - sortBy.value = [...seedSortBy.value] + const validHeaderKeys = new Set(allHeaders.value.map(({ key }) => key)) + const persistedSortBy = seedSortBy.value.filter(({ key }) => validHeaderKeys.has(key)) + sortBy.value = persistedSortBy.length ? persistedSortBy : [{ key: 'name', order: 'asc' }] + seedSortBy.value = sortBy.value } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/views/GSeedList.vue` around lines 142 - 150, Persisted seedSortBy may reference column keys that no longer exist; when restoring seedSortBy in GSeedList.vue, validate the saved key against the current headers (the array with objects containing title/key/defaultSelected, e.g., the 'shoot' header using getManagedSeedShootName). If the saved key is not found among current header.key values, discard it and fall back to a safe default (the header with defaultSelected true or an explicit fallback like 'name'), then persist that sanitized value. Implement this check where seedSortBy/localStorage is read on mount/setup so stale keys are never applied to the table.backend/lib/io/helper.js (1)
70-81: Separate the garden-scoped permissions inreq.user.profilesto preserve access control granularity.The composite flag
canGetManagedSeedAndShootInGardenNsuses AND logic, meaning a user with onlycanListManagedSeedsInGardenNamespacepermission (withoutcanListShootsInGardenNamespace) will be denied access to managed seeds in the IO layer. Bothbackend/lib/io/managedseeds.jsandbackend/lib/io/index.jscheck only the composite flag, which incorrectly locks out users who should have access to managed seeds alone.Suggested change
const profiles = Object.freeze({ canListProjects, canListSeeds, + canListManagedSeedsInGardenNamespace, + canListShootsInGardenNamespace, canGetManagedSeedAndShootInGardenNs: canListManagedSeedsInGardenNamespace && canListShootsInGardenNamespace, })Then update the access resolvers:
backend/lib/io/managedseeds.js:18→ checkcanListManagedSeedsInGardenNamespacealonebackend/lib/io/managedseedshoots.js:18→ check both permissions (keep composite or check both explicitly)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/io/helper.js` around lines 70 - 81, The composite profile flag canGetManagedSeedAndShootInGardenNs (created from canListManagedSeedsInGardenNamespace && canListShootsInGardenNamespace in the profiles object) incorrectly denies users who have only canListManagedSeedsInGardenNamespace; change profiles to expose the two boolean flags separately (keep canListManagedSeedsInGardenNamespace and canListShootsInGardenNamespace) and remove or stop relying on the composite flag for managed-seed-only checks; then update the access checks: in managedseeds.js use canListManagedSeedsInGardenNamespace alone to authorize managed-seed access, and in managedseedshoots.js continue to require both permissions (either by checking the two flags explicitly or by using the composite where appropriate).backend/__tests__/acceptance/io.spec.cjs (1)
566-571: Don’t preload access-review mocks in the JWT rejection tests.These cases should prove authentication fails before any authorization check runs. Seeding four successful
mockRequestresponses here makes the tests pass even if expired or refresh-required tokens start hitting the authz backend.♻️ Tighten the setup
beforeEach(() => { - mockRequest - .mockImplementationOnce(fixtures.auth.mocks.reviewSelfSubjectAccess()) - .mockImplementationOnce(fixtures.auth.mocks.reviewSelfSubjectAccess()) - .mockImplementationOnce(fixtures.auth.mocks.reviewSelfSubjectAccess()) - .mockImplementationOnce(fixtures.auth.mocks.reviewSelfSubjectAccess()) timestamp = Math.floor(Date.now() / 1000) })Add
expect(mockRequest).not.toHaveBeenCalled()in the two tests below so they keep asserting the auth short-circuit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/__tests__/acceptance/io.spec.cjs` around lines 566 - 571, Remove the preloaded authorization responses by deleting the four mockImplementationOnce(...fixtures.auth.mocks.reviewSelfSubjectAccess()) calls so the JWT rejection tests don't seed authz results; then, in the two JWT-rejection test cases that assert short-circuit authentication failure, add expect(mockRequest).not.toHaveBeenCalled() to ensure no authorization requests were made. Target the mockRequest setup and the two tests that assert token expiry/refresh-required behavior so they verify auth is short-circuited before any call to fixtures.auth.mocks.reviewSelfSubjectAccess().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/__tests__/acceptance/io.spec.cjs`:
- Around line 566-571: Remove the preloaded authorization responses by deleting
the four
mockImplementationOnce(...fixtures.auth.mocks.reviewSelfSubjectAccess()) calls
so the JWT rejection tests don't seed authz results; then, in the two
JWT-rejection test cases that assert short-circuit authentication failure, add
expect(mockRequest).not.toHaveBeenCalled() to ensure no authorization requests
were made. Target the mockRequest setup and the two tests that assert token
expiry/refresh-required behavior so they verify auth is short-circuited before
any call to fixtures.auth.mocks.reviewSelfSubjectAccess().
In `@backend/__tests__/services.managedseedshoots.spec.cjs`:
- Around line 39-50: Add a test that covers the inverse authorization path by
simulating canListManagedSeedsInGardenNamespace returning true but
canListShootsInGardenNamespace returning false, then call
managedseedshoots.list({ user, namespace: 'garden' }) and assert it rejects with
a 403 and the same message; also assert that
authorization.canListManagedSeedsInGardenNamespace was calledWith(user),
authorization.canListShootsInGardenNamespace was calledWith(user), and that
cache.getManagedSeedsInGardenNamespace was not called to ensure both checks are
enforced.
In `@backend/lib/io/helper.js`:
- Around line 70-81: The composite profile flag
canGetManagedSeedAndShootInGardenNs (created from
canListManagedSeedsInGardenNamespace && canListShootsInGardenNamespace in the
profiles object) incorrectly denies users who have only
canListManagedSeedsInGardenNamespace; change profiles to expose the two boolean
flags separately (keep canListManagedSeedsInGardenNamespace and
canListShootsInGardenNamespace) and remove or stop relying on the composite flag
for managed-seed-only checks; then update the access checks: in managedseeds.js
use canListManagedSeedsInGardenNamespace alone to authorize managed-seed access,
and in managedseedshoots.js continue to require both permissions (either by
checking the two flags explicitly or by using the composite where appropriate).
In `@backend/lib/watches/managedseeds.js`:
- Around line 22-24: The delete handler currently calls handleEvent('DELETED',
object) which treats the deleted resource as the "new" object; change it to pass
the deleted resource as the oldObject (i.e., call handleEvent('DELETED',
undefined, object)) so it matches the update pattern and the function signature
of handleEvent(newObject, oldObject).
In `@frontend/__tests__/composables/useApi.spec.js`:
- Around line 36-52: Add a new test block mirroring the existing
describe('#getManagedSeedShootsForGardenNamespace') case to cover
getManagedSeedsForGardenNamespace: mock fetch response with
fetch.mockResponseOnce(JSON.stringify([]), { headers: { 'content-type':
'application/json; charset=UTF-8' } }), call
api.getManagedSeedsForGardenNamespace(), assert res.status === 200 and res.data
equals [], assert fetch called once, and assert the requested URL equals
'http://localhost:3000/api/namespaces/garden/managedseeds' so the new function
getManagedSeedsForGardenNamespace is covered.
In `@frontend/src/store/managedSeedShoot.js`:
- Around line 50-52: The helper shootByName currently calls find(list.value, {
metadata: { namespace, name } }) which can receive null when list is
uninitialized; add an explicit null-safety guard in the shootByName function
(check list.value or isInitial) and return null/undefined early if the list is
not ready so callers see the intent clearly; reference the shootByName function
and the list.value access and use an early return to avoid relying on implicit
lodash behavior.
In `@frontend/src/views/GSeedList.vue`:
- Around line 142-150: Persisted seedSortBy may reference column keys that no
longer exist; when restoring seedSortBy in GSeedList.vue, validate the saved key
against the current headers (the array with objects containing
title/key/defaultSelected, e.g., the 'shoot' header using
getManagedSeedShootName). If the saved key is not found among current header.key
values, discard it and fall back to a safe default (the header with
defaultSelected true or an explicit fallback like 'name'), then persist that
sanitized value. Implement this check where seedSortBy/localStorage is read on
mount/setup so stale keys are never applied to the table.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a9327e0-d28a-4e19-9c0d-b49d341a70a6
⛔ Files ignored due to path filters (2)
backend/__tests__/acceptance/__snapshots__/api.managedseeds.spec.cjs.snapis excluded by!**/*.snapcharts/__tests__/gardener-dashboard/application/dashboard/__snapshots__/clusterrole.spec.js.snapis excluded by!**/*.snap
📒 Files selected for processing (49)
backend/__tests__/acceptance/api.managedseeds.spec.cjsbackend/__tests__/acceptance/io.cors.spec.cjsbackend/__tests__/acceptance/io.spec.cjsbackend/__tests__/hooks.spec.cjsbackend/__tests__/services.managedseeds.spec.cjsbackend/__tests__/services.managedseedshoots.spec.cjsbackend/__tests__/watches.spec.cjsbackend/lib/cache/index.jsbackend/lib/hooks.jsbackend/lib/io/dispatcher.jsbackend/lib/io/helper.jsbackend/lib/io/index.jsbackend/lib/io/managedseeds.jsbackend/lib/io/managedseedshoots.jsbackend/lib/routes/index.jsbackend/lib/routes/managedseeds.jsbackend/lib/routes/managedseedshoots.jsbackend/lib/services/authorization.jsbackend/lib/services/index.jsbackend/lib/services/managedseeds.jsbackend/lib/services/managedseedshoots.jsbackend/lib/utils/index.jsbackend/lib/watches/index.jsbackend/lib/watches/managedseeds.jsbackend/lib/watches/shoots.jscharts/gardener-dashboard/charts/application/templates/dashboard/clusterrole.yamlfrontend/__tests__/composables/useApi.spec.jsfrontend/src/components/GManagedSeedShootLink.vuefrontend/src/components/GSeedListRow.vuefrontend/src/components/GSeedStatusTags.vuefrontend/src/components/GShootListRow.vuefrontend/src/components/SeedDetails/GSeedDetailsCard.vuefrontend/src/components/SeedDetails/GSeedMonitoringCard.vuefrontend/src/composables/useApi/api.jsfrontend/src/composables/useManagedSeedShootForSeed.jsfrontend/src/composables/useSeedEffectiveConditions.jsfrontend/src/composables/useSeedItem/helper.jsfrontend/src/composables/useSeedItem/index.jsfrontend/src/composables/useSeedTableSorting.jsfrontend/src/composables/useShootAdvertisedAddresses.jsfrontend/src/router/guards.jsfrontend/src/store/managedSeed.jsfrontend/src/store/managedSeedShoot.jsfrontend/src/store/socket/helper.jsfrontend/src/store/socket/index.jsfrontend/src/views/GSeedItemPlaceholder.vuefrontend/src/views/GSeedList.vuefrontend/src/views/GShootItemPlaceholder.vuefrontend/src/views/GShootList.vue
💤 Files with no reviewable changes (1)
- frontend/src/composables/useSeedItem/helper.js
petersutter
force-pushed
the
enh/link-to-managed-seed-shoot
branch
from
3533048 to
cea4f92
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
frontend/src/components/SeedDetails/GSeedMonitoringCard.vue (1)
59-72: Consider adding icons for visual consistency.The existing Plutono tile (line 44) specifies
icon="mdi-developer-board", but the new Shoot Plutono and Shoot Prometheus tiles omit the icon prop. This creates visual inconsistency within the card.If the omission is intentional (to visually subordinate these links), this is fine. Otherwise, consider adding appropriate icons.
♻️ Suggested change
<g-link-list-tile v-if="managedSeedShootPlutonoUrl" + icon="mdi-developer-board" app-title="Shoot Plutono" :url="managedSeedShootPlutonoUrl" :url-text="managedSeedShootPlutonoUrl" content-class="pt-0" /> <g-link-list-tile v-if="managedSeedShootPrometheusUrl" + icon="mdi-chart-line" app-title="Shoot Prometheus" :url="managedSeedShootPrometheusUrl" :url-text="managedSeedShootPrometheusUrl" content-class="pt-0" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/SeedDetails/GSeedMonitoringCard.vue` around lines 59 - 72, The two g-link-list-tile components rendering Shoot Plutono and Shoot Prometheus (using managedSeedShootPlutonoUrl and managedSeedShootPrometheusUrl) omit the icon prop causing visual inconsistency with the existing Plutono tile; update those components to pass an appropriate icon prop (e.g., icon="mdi-developer-board" or another suitable mdi-* icon) to match the other tiles, or if omission was intentional, add a brief comment above each g-link-list-tile explaining the deliberate visual subordination.
🤖 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/watches/shoots.js`:
- Around line 55-58: The current delete/update handler returns early when
cache.getManagedSeedForShootInGardenNamespace(name) yields no managedSeed,
causing missed DELETED events; change the logic so that when managedSeed is
undefined you still emit cleanup DELETED events to the
"managedseed-shoots;garden" clients based on a persisted tracking structure
(e.g., a previouslyPublishedManagedSeedShoots map keyed by shoot name or UID)
and then remove the entry from that tracking map; update the code around the
managedSeed variable usage to consult this map on missing lookups, publish the
DELETED event for previously-associated clients, and clear the tracked record so
stale entries cannot remain.
In `@frontend/src/composables/useSeedEffectiveConditions.js`:
- Around line 23-27: The code currently treats only undefined as missing and
returns managedSeedShootConditions.value even when it's null; update the check
in useSeedEffectiveConditions (or replace the conditional) so that null is
treated as missing too — e.g., check for non-nullish value
(managedSeedShootConditions.value != null) or use the nullish coalescing
behavior to return seedConditions.value when managedSeedShootConditions.value is
null or undefined, ensuring downstream rendering gets seedConditions.value
instead of null.
In `@frontend/src/views/GSeedList.vue`:
- Around line 142-150: The SHOOT column's sort key currently falls back to an
empty string which mismatches the displayed unmanaged label; update the value
fallback in the column definition (the object with title 'SHOOT' and value: item
=> getManagedSeedShootName(item) || '') to use 'Unmanaged' instead of '', and
make the same change wherever getManagedSeedShootName(item) is used for
sorting/display (the other occurrence referenced around lines 249-253) so
sorting/search uses the same 'Unmanaged' text shown to users.
---
Nitpick comments:
In `@frontend/src/components/SeedDetails/GSeedMonitoringCard.vue`:
- Around line 59-72: The two g-link-list-tile components rendering Shoot Plutono
and Shoot Prometheus (using managedSeedShootPlutonoUrl and
managedSeedShootPrometheusUrl) omit the icon prop causing visual inconsistency
with the existing Plutono tile; update those components to pass an appropriate
icon prop (e.g., icon="mdi-developer-board" or another suitable mdi-* icon) to
match the other tiles, or if omission was intentional, add a brief comment above
each g-link-list-tile explaining the deliberate visual subordination.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 273a679c-19fd-4769-a581-9865d2d88ece
⛔ Files ignored due to path filters (2)
backend/__tests__/acceptance/__snapshots__/api.managedseeds.spec.cjs.snapis excluded by!**/*.snapcharts/__tests__/gardener-dashboard/application/dashboard/__snapshots__/clusterrole.spec.js.snapis excluded by!**/*.snap
📒 Files selected for processing (50)
backend/__tests__/acceptance/api.managedseeds.spec.cjsbackend/__tests__/acceptance/io.cors.spec.cjsbackend/__tests__/acceptance/io.spec.cjsbackend/__tests__/hooks.spec.cjsbackend/__tests__/services.managedseeds.spec.cjsbackend/__tests__/services.managedseedshoots.spec.cjsbackend/__tests__/watches.spec.cjsbackend/lib/cache/index.jsbackend/lib/hooks.jsbackend/lib/io/dispatcher.jsbackend/lib/io/helper.jsbackend/lib/io/index.jsbackend/lib/io/managedseeds.jsbackend/lib/io/managedseedshoots.jsbackend/lib/routes/index.jsbackend/lib/routes/managedseeds.jsbackend/lib/routes/managedseedshoots.jsbackend/lib/services/authorization.jsbackend/lib/services/index.jsbackend/lib/services/managedseeds.jsbackend/lib/services/managedseedshoots.jsbackend/lib/utils/index.jsbackend/lib/watches/index.jsbackend/lib/watches/managedseeds.jsbackend/lib/watches/shoots.jscharts/gardener-dashboard/charts/application/templates/dashboard/clusterrole.yamlfrontend/__tests__/composables/useApi.spec.jsfrontend/src/components/GManagedSeedShootLink.vuefrontend/src/components/GSeedListRow.vuefrontend/src/components/GSeedStatusTags.vuefrontend/src/components/GShootListRow.vuefrontend/src/components/SeedDetails/GSeedDetailsCard.vuefrontend/src/components/SeedDetails/GSeedInfrastructureCard.vuefrontend/src/components/SeedDetails/GSeedMonitoringCard.vuefrontend/src/composables/useApi/api.jsfrontend/src/composables/useManagedSeedShootForSeed.jsfrontend/src/composables/useSeedEffectiveConditions.jsfrontend/src/composables/useSeedItem/helper.jsfrontend/src/composables/useSeedItem/index.jsfrontend/src/composables/useSeedTableSorting.jsfrontend/src/composables/useShootAdvertisedAddresses.jsfrontend/src/router/guards.jsfrontend/src/store/managedSeed.jsfrontend/src/store/managedSeedShoot.jsfrontend/src/store/socket/helper.jsfrontend/src/store/socket/index.jsfrontend/src/views/GSeedItemPlaceholder.vuefrontend/src/views/GSeedList.vuefrontend/src/views/GShootItemPlaceholder.vuefrontend/src/views/GShootList.vue
💤 Files with no reviewable changes (2)
- frontend/src/composables/useSeedItem/helper.js
- frontend/src/components/SeedDetails/GSeedInfrastructureCard.vue
🚧 Files skipped from review as they are similar to previous changes (29)
- frontend/src/views/GShootList.vue
- backend/lib/services/authorization.js
- backend/lib/io/index.js
- backend/lib/routes/managedseeds.js
- backend/lib/io/managedseedshoots.js
- backend/lib/routes/managedseedshoots.js
- frontend/src/components/GShootListRow.vue
- backend/lib/utils/index.js
- backend/tests/watches.spec.cjs
- frontend/src/components/GManagedSeedShootLink.vue
- backend/lib/io/managedseeds.js
- backend/tests/services.managedseedshoots.spec.cjs
- frontend/src/store/socket/index.js
- backend/lib/io/helper.js
- frontend/src/store/socket/helper.js
- backend/tests/acceptance/api.managedseeds.spec.cjs
- backend/lib/watches/index.js
- backend/lib/services/index.js
- frontend/src/store/managedSeedShoot.js
- frontend/src/store/managedSeed.js
- frontend/src/composables/useManagedSeedShootForSeed.js
- backend/lib/routes/index.js
- frontend/tests/composables/useApi.spec.js
- frontend/src/views/GSeedItemPlaceholder.vue
- frontend/src/composables/useSeedItem/index.js
- frontend/src/router/guards.js
- frontend/src/composables/useShootAdvertisedAddresses.js
- backend/lib/io/dispatcher.js
- backend/lib/services/managedseedshoots.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/watches/shoots.js`:
- Around line 49-71: publishManagedSeedShoots currently drops ADDED/MODIFIED
shoot events if cache.getManagedSeedForShootInGardenNamespace(name) returns
falsy, causing ordering/staleness; instead, stop treating the cache as
authoritative: either (A) always emit the shoot event to the
'managedseed-shoots;garden' room (like DELETED does) so clients receive the
initial ADDED/MODIFIED, or (B) implement a backfill in the ManagedSeed watcher:
in backend/lib/watches/managedseeds.js subscribe to ManagedSeed ADDED/MODIFIED
and when a ManagedSeed appears/changes, look up its shoot reference and emit the
corresponding managedseed-shoots message (use the same payload shape and the
publishManagedSeedShoots helper or a new emit helper) so associations discovered
after the Shoot event still notify clients; update publishManagedSeedShoots to
remove the early return (or record pending shoots) and ensure cache lookups are
only used for enrichment, not gating emission.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d636adb-c929-4dda-861f-b061f114b417
📒 Files selected for processing (2)
backend/__tests__/watches.spec.cjsbackend/lib/watches/shoots.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/__tests__/watches.spec.cjs (1)
212-236: Consider verifying cache method call count.The test correctly verifies the emitted events, but it could also assert that
getManagedSeedForShootInGardenNamespacewas called exactly 2 times (for ADD and UPDATE only, not DELETE) to fully verify the conditional logic in the production code.💡 Optional enhancement
expect(room.emit.mock.calls).toEqual([ ['managedseed-shoots', { type: 'ADDED', uid: gardenManagedSeedShoot.metadata.uid }], ['managedseed-shoots', { type: 'MODIFIED', uid: gardenManagedSeedShoot.metadata.uid }], ['managedseed-shoots', { type: 'DELETED', uid: gardenManagedSeedShoot.metadata.uid }], ]) + + // Verify cache lookup was skipped for DELETE + expect(cache.getManagedSeedForShootInGardenNamespace).toHaveBeenCalledTimes(2) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/__tests__/watches.spec.cjs` around lines 212 - 236, Add an assertion to verify the cache call count: after emitting the events and awaiting flushPromises in the "should emit managed seed shoots to a dedicated garden room" test, assert that cache.getManagedSeedForShootInGardenNamespace was called exactly 2 times (only for ADD and UPDATE). Use the existing mock name getManagedSeedForShootInGardenNamespace and place the expect(cache.getManagedSeedForShootInGardenNamespace).toHaveBeenCalledTimes(2) alongside the other expects that check room.emit to ensure the conditional logic is fully validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/__tests__/watches.spec.cjs`:
- Around line 212-236: Add an assertion to verify the cache call count: after
emitting the events and awaiting flushPromises in the "should emit managed seed
shoots to a dedicated garden room" test, assert that
cache.getManagedSeedForShootInGardenNamespace was called exactly 2 times (only
for ADD and UPDATE). Use the existing mock name
getManagedSeedForShootInGardenNamespace and place the
expect(cache.getManagedSeedForShootInGardenNamespace).toHaveBeenCalledTimes(2)
alongside the other expects that check room.emit to ensure the conditional logic
is fully validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd4a1f88-2045-4143-8b47-67e7e513f810
📒 Files selected for processing (2)
backend/__tests__/watches.spec.cjsbackend/lib/watches/managedseeds.js
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/lib/watches/managedseeds.js
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
Tests
Refactor