feat: use kubernetes dashboard api #121
Conversation
feloy
requested review from
axel7083 and
simonrey1
and removed request for
a team
feloy
force-pushed
the
chore/use-dashboard-api
branch
from
5fa3d5c to
9f6fd32
Compare
|
Warning Rate limit exceeded@feloy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughIntroduces a new DashboardStatesManager class that lazily acquires and manages the Kubernetes Dashboard extension API subscriber. The manager is integrated into the extension's dependency injection container and lifecycle. Supporting mocks, tests, and a new development dependency are added to support the feature. Changes
Sequence DiagramsequenceDiagram
participant CE as ContextsExtension
participant DSM as DashboardStatesManager
participant EA as extensions API
participant KDEA as redhat.kubernetes-dashboard<br/>(KubernetesDashboardExtensionApi)
participant KDS as KubernetesDashboardSubscriber
CE->>DSM: init()
DSM->>EA: onDidChange(listener)
Note over DSM: Listener registered,<br/>awaiting extension availability
rect rgb(200, 220, 255)
Note over KDEA,KDS: Extension becomes available
EA->>DSM: onDidChange callback
DSM->>EA: getExtension('redhat.kubernetes-dashboard')
EA-->>DSM: extension found
DSM->>KDEA: extension.exports
KDEA-->>DSM: KubernetesDashboardSubscriber
DSM->>DSM: Store subscriber
Note over DSM: Subscriber now accessible<br/>via getSubscriber()
end
rect rgb(255, 220, 200)
Note over CE: Cleanup phase
CE->>DSM: dispose()
DSM->>DSM: Iterate subscriptions
DSM->>EA: onDidChangeDisposable.dispose()
DSM->>KDS: subscriber?.dispose()
Note over DSM: All resources cleaned up
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks✅ Passed checks (3 passed)
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: 2
🧹 Nitpick comments (4)
packages/extension/src/manager/_manager-module.ts (1)
24-24: Typo in filename:dashoboardshould bedashboard.The import path contains a typo. Consider renaming the file to
dashboard-states-manager.tsfor consistency with the test file naming (dashboard-states-manager.spec.ts).-import { DashboardStatesManager } from '/@/manager/dashoboard-states-manager'; +import { DashboardStatesManager } from '/@/manager/dashboard-states-manager';packages/extension/src/contexts-extension.ts (1)
33-33: Typo in import path:dashoboardshould bedashboard.Same typo as noted in
_manager-module.ts. This will need to be updated once the file is renamed.packages/extension/src/manager/dashboard-states-manager.spec.ts (1)
20-20: Typo in import path.Consistent with other files - update after renaming the source file.
packages/extension/src/manager/dashoboard-states-manager.ts (1)
26-27: Typo in filename:dashoboard-states-manager.tsshould bedashboard-states-manager.ts.The filename contains a typo. This should be renamed to match the test file naming convention (
dashboard-states-manager.spec.ts).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
__mocks__/@podman-desktop/api.js(1 hunks)packages/extension/package.json(1 hunks)packages/extension/src/contexts-extension.ts(3 hunks)packages/extension/src/manager/_manager-module.ts(1 hunks)packages/extension/src/manager/dashboard-states-manager.spec.ts(1 hunks)packages/extension/src/manager/dashoboard-states-manager.ts(1 hunks)
🔇 Additional comments (6)
packages/extension/src/manager/_manager-module.ts (1)
30-30: LGTM!The singleton binding follows the established pattern for other managers in this module.
__mocks__/@podman-desktop/api.js (1)
87-90: LGTM!The mock additions for
extensions.onDidChangeandextensions.getExtensionalign with the API surface used byDashboardStatesManagerand follow the existing mock patterns in this file.packages/extension/package.json (1)
27-27: Verify the use of"next"version tag.Using
"next"as the version tag means this dependency will receive the latest pre-release version on each install, which could introduce unexpected breaking changes. Consider whether a pinned version would be more appropriate for stability, or confirm this is intentional for tracking the latest API during development.packages/extension/src/contexts-extension.ts (1)
67-69: LGTM!The initialization flow correctly retrieves the manager from the IoC container, initializes it, and registers it for disposal via the extension subscriptions.
packages/extension/src/manager/dashboard-states-manager.spec.ts (1)
27-53: LGTM!The test suite for the "not installed" scenario properly validates that the subscriber is undefined and that disposal cleanup occurs correctly.
packages/extension/src/manager/dashoboard-states-manager.ts (1)
46-55: LGTM!The
dispose()method correctly cleans up all subscriptions, andgetSubscriber()provides appropriate read access to the subscriber.
feloy
force-pushed
the
chore/use-dashboard-api
branch
from
9f6fd32 to
58145d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/extension/src/manager/dashoboard-states-manager.ts (1)
31-44: Missing initial check for already-installed extension.This issue was already flagged in a previous review. The
init()method only registers theonDidChangelistener but doesn't check if the extension is already installed. If the dashboard extension is installed beforeinit()is called, the subscriber won't be obtained until another extension change event fires.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
__mocks__/@podman-desktop/api.js(1 hunks)packages/extension/package.json(1 hunks)packages/extension/src/contexts-extension.ts(3 hunks)packages/extension/src/manager/_manager-module.ts(1 hunks)packages/extension/src/manager/dashboard-states-manager.spec.ts(1 hunks)packages/extension/src/manager/dashoboard-states-manager.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/extension/package.json
- packages/extension/src/manager/_manager-module.ts
- packages/extension/src/manager/dashboard-states-manager.spec.ts
- packages/extension/src/contexts-extension.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build extension OCI image
🔇 Additional comments (1)
__mocks__/@podman-desktop/api.js (1)
87-90: LGTM!The
extensionsmock is correctly structured to support the newDashboardStatesManagertests, consistent with the existing mock patterns in this file.
feloy
force-pushed
the
chore/use-dashboard-api
branch
from
58145d2 to
68bb0cf
Compare
feloy
force-pushed
the
chore/use-dashboard-api
branch
from
68bb0cf to
db8e487
Compare
|
I am not sure how to review this PR, might not be the best person to do it sorry. |
If it can help the review, here the complete context of the PR: Podman Desktop introduced recently the possibility for extensions to provide an API (we started with the podman and docker extensions, for example podman-desktop/podman-desktop#5990). The Kubernetes Dashboard extension, which is responsible for getting the states of the different contexts, uses this pattern to share these states with other extensions (podman-desktop/extension-kubernetes-dashboard#445). The use of the API is documented in the code added by this PR (https://github.com/podman-desktop/extension-kubernetes-dashboard/blob/37b6d0c6f12b9ef1604683fc1802c604cf7dce4c/packages/api/src/kubernetes-dashboard-extension-api.d.ts#L96-L117) The Contexts extension needs to get access to the contexts states (reachable, number of pods/deployments), to be able to display this information in the left part of the Context information. 
This first PR tries to access the extension API. Note that the Dashboard extension is not necessary for the Contexts extension to run correctly. If the Dashboard is not installed, the Contexts list won't display the state of the context, but only the data in the kubeconfig file. |
Signed-off-by: Philippe Martin <phmartin@redhat.com>
feloy
force-pushed
the
chore/use-dashboard-api
branch
from
db8e487 to
516fd2a
Compare
simonrey1
left a comment
simonrey1
left a comment
There was a problem hiding this comment.
The code added here look similar to what is documented there:
The extension seems to still be running OK with this change.
|
|
||
| beforeEach(() => { | ||
| vi.mocked(extensions.onDidChange).mockImplementation(f => { | ||
| setTimeout(() => { |
There was a problem hiding this comment.
question: Why is it necessary to use setTimeout here?
There was a problem hiding this comment.
Without this, const didChangeSubscription = extensions.onDidChange is not executed yet when the callback is executed, and didChangeSubscription.dispose() fails in the callback because didChangeSubscription is undefined
| Disposable: { | ||
| create: (func) => ({ dispose: func }), | ||
| }, | ||
| extensions: { |
There was a problem hiding this comment.
question: the update of __mocks__/@podman-desktop/api.jsseems to be more an upstream fix than just here?
There was a problem hiding this comment.
Yes, I'll need to use the auto-generated mock used in other extensions. In the meantime, I'm adding the methods used when necessary
3 participants
Part of #21
On activation, the extension tries to connect to the Kubernetes Dashboard extension, to get its subscriber