Add CRD detection guard and unit tests for Agones views#12
Add CRD detection guard and unit tests for Agones views#12NAME-ASHWANIYADAV wants to merge 6 commits into
Conversation
NAME-ASHWANIYADAV
force-pushed
the
feat/crd-detection-guard
branch
from
13b713b to
b33c7f0
Compare
| import { useEffect, useState } from 'react'; | ||
| import { isAgonesInstalled as checkAgonesInstallation } from '../isAgonesInstalled'; | ||
|
|
||
| export function useAgonesInstalled() { |
There was a problem hiding this comment.
I have added full TSDoc documentation for the hook with a description of the return object and a usage example👍
| */ | ||
|
|
||
| import { useEffect, useState } from 'react'; | ||
| import { isAgonesInstalled as checkAgonesInstallation } from '../isAgonesInstalled'; |
There was a problem hiding this comment.
Thank for pointing this , I have removed it
The function is now called directly as isAgonesInstalled()
|
|
||
| import { ApiProxy } from '@kinvolk/headlamp-plugin/lib'; | ||
|
|
||
| export async function isAgonesInstalled(): Promise<boolean> { |
There was a problem hiding this comment.
Could this make more sense inside the useAgonesInstalled? I don't think it will be used anywhere else.
There was a problem hiding this comment.
I have moved the entire isAgonesInstalled() function into useAgonesInstalled.tsx and deleted this standalone file
I also clean up the debug console.log statements and update the mock strategy in the tests to work with this new structure
|
@illume @markmandel @lacroixthomas PTAL !! |
| component: () => ( | ||
| <AgonesInstallCheck> | ||
| <AgonesOverview /> | ||
| </AgonesInstallCheck> | ||
| ), |
There was a problem hiding this comment.
I'm pretty rusty about react, feel free to ignore me
I was wondering if it's a good pattern to have some kind of helper / wrapper for that as it's duplicated each time ? This way if we add some specific error banner or something it could be done only there ? 🤔
Something along these lines ?
const withInstallCheck = (Component: React.ComponentType) => () => (
<AgonesInstallCheck>
<Component />
</AgonesInstallCheck>
)
then component: withInstallCheck(AgonesOverview)
WDYT ?
There was a problem hiding this comment.
Hey @lacroixthomas! I have implemented the withInstallCheck HOC exactly as you suggested and refactored all the routes in src/index.tsx to use it
I also took the opportunity to resolve the recent merge conflicts in package.json caused by Dependabot, and rebased the branch to keep the history clean.
Thanks for the review! 🙏
Signed-off-by: ashwani yadav <22ashwaniyadav@gmail.com>
Signed-off-by: ashwani yadav <22ashwaniyadav@gmail.com>
Signed-off-by: ashwani yadav <22ashwaniyadav@gmail.com>
Signed-off-by: ashwani yadav <22ashwaniyadav@gmail.com>
Signed-off-by: ashwani yadav <22ashwaniyadav@gmail.com>
NAME-ASHWANIYADAV
force-pushed
the
feat/crd-detection-guard
branch
from
a09a9a1 to
da417a9
Compare
The CRD detecting guard is now working properly 🎉🎉
will appreciate a re-review @illume @lacroixthomas @markmandel
|
|
@markmandel plz re-run ci |
Signed-off-by: ashwani yadav <22ashwaniyadav@gmail.com>
NAME-ASHWANIYADAV
force-pushed
the
feat/crd-detection-guard
branch
from
7e26a22 to
a450497
Compare
|
@markmandel i have fixed the lint and formatting issues - could you approve the CI run ? Thanks! 🙏 |
|
Oh fun - there is the artifact download too. We should take it for a spin! |
|
Perfect! Let me know if everything works as expected |
4 participants
Type of change
What this PR does
This PR introduces a robust detection mechanism to verify if the Agones Custom Resource Definitions (CRDs) are installed on the active Kubernetes cluster before attempting to render Agones-specific UI views.
Key additions:
isAgonesInstalledutility: Queries the/apis/agones.dev/v1endpoint and explicitly validates that the response is a valid KubernetesAPIResourceList.useAgonesInstalledhook: A React hook to manage the asynchronous loading state.AgonesInstallCheckwrapper component: Intercepts routes and is designed to display a helpful banner if the API is missing.src/index.tsxare wrapped with this component.Screenshots / recordings
(I am currently facing a UI rendering issue locally. Attached are the screenshots showing
minikubevstest-no-agoneswhere the UI still renders the default view instead of the fallback banner).Test plan
Automated checks
npm cinpm run buildnpm run tscnpm run lintnpm run format -- --checknpm run testManual verification
minikube)test-no-agones)(See Notes for Reviewers regarding the local UI verification issue).
RBAC considerations
No additional RBAC permissions are required beyond what the plugin already needs.
Notes for reviewers
I have successfully implemented the logic, wrapped the components in
index.tsx, and all automated unit tests pass perfectly.However, when I test this locally on the Headlamp Desktop App (using my
test-no-agonescluster), I am facing a weird issue: TheisAgonesInstalledcheck should catch the missing API, but the UI still renders the defaultAgonesOverviewcomponent instead of myNotInstalledBanner(as seen in the second screenshot).I suspect this might be related to how
registerRoutehandles wrapped components in Headlamp, or perhaps UI caching in the Desktop app (though I have cleared the.pluginscache).I've opened this PR so you can review the code structure. Any pointers on why the route wrapper isn't displaying the fallback banner would be hugely appreciated!