-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Feat/infra monitoring k8s volume #6900
Merged
amlannandy
merged 4 commits into
feat/infra-monitoring-k8s-jobs
from
feat/infra-monitoring-k8s-volume
Jan 22, 2025
Merged
Feat/infra monitoring k8s volume #6900
amlannandy
merged 4 commits into
feat/infra-monitoring-k8s-jobs
from
feat/infra-monitoring-k8s-volume
Jan 22, 2025
Conversation
This file contains 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
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.
👍 Looks good to me! Reviewed everything up to 6fa7e39 in 2 minutes and 18 seconds
More details
- Looked at
5600
lines of code in41
files - Skipped
0
files when reviewing. - Skipped posting
9
drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringK8s/Volumes/VolumeDetails/VolumeDetails.tsx:34
- Draft comment:
The conversion factor for nanoseconds to milliseconds should be 1000000, not 1000000000. Please update the conversion logic accordingly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
- To convert from nanoseconds to milliseconds, we need to divide by 1,000,000 (1e6) since 1ms = 1,000,000ns. 2. However, looking at the variable names (startMs, endMs), these might actually be meant to be in seconds, not milliseconds. 3. To convert from nanoseconds to seconds, we divide by 1,000,000,000 (1e9). 4. The current code uses 1e9 consistently, suggesting it's intentionally converting to seconds.
I could be wrong about the intended unit - maybe these times really should be in milliseconds. Without seeing how these values are used by other components, I can't be 100% certain.
The consistent use of 1e9 throughout the file and the fact that Unix timestamps are typically in seconds strongly suggests these are meant to be second-based timestamps, not milliseconds.
The comment appears to be incorrect. The code is likely intentionally converting to seconds, not milliseconds, so the 1000000000 divisor is correct.
2. frontend/src/container/InfraMonitoringK8s/Volumes/VolumeDetails/VolumeDetails.tsx:37
- Draft comment:
The conversion factor for nanoseconds to milliseconds should be 1000000, not 1000000000. Please update the conversion logic accordingly. - Reason this comment was not posted:
Marked as duplicate.
3. frontend/src/container/InfraMonitoringK8s/Volumes/VolumeDetails/VolumeDetails.tsx:66
- Draft comment:
The conversion factor for nanoseconds to milliseconds should be 1000000, not 1000000000. Please update the conversion logic accordingly. - Reason this comment was not posted:
Marked as duplicate.
4. frontend/src/container/InfraMonitoringK8s/Volumes/VolumeDetails/VolumeDetails.tsx:67
- Draft comment:
The conversion factor for nanoseconds to milliseconds should be 1000000, not 1000000000. Please update the conversion logic accordingly. - Reason this comment was not posted:
Marked as duplicate.
5. frontend/src/container/InfraMonitoringK8s/Volumes/VolumeDetails/VolumeDetails.tsx:85
- Draft comment:
The conversion factor for nanoseconds to milliseconds should be 1000000, not 1000000000. Please update the conversion logic accordingly. - Reason this comment was not posted:
Marked as duplicate.
6. frontend/src/container/InfraMonitoringK8s/Volumes/VolumeDetails/VolumeDetails.tsx:86
- Draft comment:
The conversion factor for nanoseconds to milliseconds should be 1000000, not 1000000000. Please update the conversion logic accordingly. - Reason this comment was not posted:
Marked as duplicate.
7. frontend/src/container/InfraMonitoringK8s/Volumes/VolumeDetails/VolumeDetails.tsx:106
- Draft comment:
The conversion factor for nanoseconds to milliseconds should be 1000000, not 1000000000. Please update the conversion logic accordingly. - Reason this comment was not posted:
Marked as duplicate.
8. frontend/src/container/InfraMonitoringK8s/Volumes/VolumeDetails/VolumeDetails.tsx:107
- Draft comment:
The conversion factor for nanoseconds to milliseconds should be 1000000, not 1000000000. Please update the conversion logic accordingly. - Reason this comment was not posted:
Marked as duplicate.
9. frontend/src/container/InfraMonitoringK8s/Volumes/utils.tsx:137
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values for consistency. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_B3GVR8qJm1sc3yPz
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
amlannandy
force-pushed
the
feat/infra-monitoring-k8s-volume
branch
from
January 22, 2025 11:31
6fa7e39
to
d3da712
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Related Issues / PR's
Screenshots
NA
Affected Areas and Manually Tested Areas
Important
Add Kubernetes volumes monitoring feature with new API calls, UI components, styles, and utilities.
getK8sVolumesList
function ingetK8sVolumesList.ts
to fetch Kubernetes volumes data.getK8sDaemonSetsList.ts
,getK8sJobsList.ts
, andgetsK8sStatefulSetsList.ts
to useaxios
.K8sVolumesList
component inK8sVolumesList.tsx
for displaying volumes.VolumeDetails
component inVolumeDetails.tsx
for detailed view of a volume.InfraMonitoringK8s.tsx
to include volumes in the monitoring categories.K8sVolumesList.styles.scss
andentityDetails.styles.scss
.utils.tsx
for handling volume data.constants.ts
to include volume-related configurations and mappings.useGetK8sVolumesList
hook inuseGetK8sVolumesList.ts
for fetching volume data using React Query.This description was created by for 6fa7e39. It will automatically update as commits are pushed.