-
-
Notifications
You must be signed in to change notification settings - Fork 99
Add CPU and Memory columns #181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
bilkoua
commented
Feb 23, 2025
Hi @b17k0 and thank you for your contribute! Actually I see only the unit tests failing. Thanks |
HI, guys! Yes, functionally everything works as expected. However, I rushed a bit with the PR. I still need to fix the unit tests, and I can already see some areas for optimization. Unfortunately, I’ll only have time to work on this over the weekend. Also, my TypeScript coding skills are far from perfect. There are some convenient features in the proprietary version that are missing in freelens, and this is just an attempt by a non-professional to bring something I’d love to have in freelens. If someone with more TS experience is interested, we could do this together. I’m sure the result would be much better that way! |
Hi @b17k0 thank you! |
Yes of course, it already was on my list, I'll take a look tonight |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Your classes contain too many elements. I suggest creating separate files for the non-injectable component (
NonInjectablePodMetricsMemoryContent
) and the injectable one (InjectablePodMetricsMemoryContent
).NonInjectablePodMetricsMemoryContent
should only contain the component logic: HTML elements, functions, variables, and so on, whileInjectablePodMetricsMemoryContent
should only return the component wrapped ingetInjectable
, as you already did. You can takepod-logs-menu.tsx
andpod-logs-menu.injectable.ts
as examples. -
Some functions are duplicated:
calculatePodKubeMetrics
andgetPodKubeMetrics
. Could you refactor them into a separate file and call them where needed? I suggest using a hook that exposes these two functions, something you can use like this:const kubeMetrics = useKubeMetrics(); kubeMetrics.getPodKubeMetrics();
-
The functions
getPodKubeMetricsMemoryDisplayValue
andgetPodKubeMetricsCpuDisplayValue
are declared outside thePodMetricsMemoryContent
andPodMetricsCpuContent
declaration, respectively. I would move these functions inside them. -
Could you please add a unit test to verify that everything is working correctly? Unit tests are very useful to prevent regressions when someone else updates the code.
These might seem like many points, but they are just small improvements that can make the code more readable, in my opinion 😄. Please reach me out if you need help or if you disagree with something, and we can discuss it.
Does it crash if metrics-server is missing in the cluster? |