-
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: implement nodes list table in infra monitoring #6615
base: feat/infra-monitoring-k8s
Are you sure you want to change the base?
feat: implement nodes list table in infra monitoring #6615
Conversation
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
@@ -14,6 +14,13 @@ import { | |||
} from 'components/QuickFilters/QuickFilters'; | |||
import { DataTypes } from 'types/api/queryBuilder/queryAutocompleteResponse'; | |||
|
|||
export interface IEntityColumn { |
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.
Using this instead of IPodColumn
. As we're reusing it, thought it should have a more generic name.
We can remove IPodColumn
in the end. I didn't rename it now to avoid conflicts.
@@ -127,4 +128,11 @@ function K8sHeader({ | |||
); | |||
} | |||
|
|||
K8sHeader.defaultProps = { |
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.
Made these fields optional for list views where there is no metadata to show
ea32f5e
to
0bbd4d0
Compare
f8b95d1
to
efd5c2c
Compare
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
1 similar comment
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
❌ Changes requested. Reviewed everything up to 8a1558a in 2 minutes and 7 seconds
More details
- Looked at
1006
lines of code in9
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringK8s/K8sFiltersSidePanel/K8sFiltersSidePanel.tsx:129
- Draft comment:
Duplicate default export. Remove one of theexport default K8sFiltersSidePanel;
statements. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment claims there are duplicate default exports, but I can only see one export default statement in the file. The comment appears to be incorrect. The code follows a common pattern of defining the component, setting its defaultProps, and then exporting it once at the end.
Could there be another export default statement that was removed in this diff that makes this comment relevant to the changes?
Looking at the diff, I can see that the original code did have an export default on the component definition that was removed in favor of the new pattern. However, in the current state of the code, there is no duplicate export.
The comment is incorrect for the current state of the code - there is no duplicate export default. The code has been properly refactored to use a single export.
2. frontend/src/container/InfraMonitoringK8s/K8sHeader.tsx:161
- Draft comment:
Duplicate default export. Remove one of theexport default K8sHeader;
statements. - Reason this comment was not posted:
Comment was on unchanged code.
3. frontend/src/container/InfraMonitoringK8s/Nodes/utils.tsx:82
- Draft comment:
TheorderBy
field uses 'cpu' which does not match any field inK8sNodesData
. Ensure the column name is correct. - Reason this comment was not posted:
Comment did not seem useful.
4. frontend/src/container/InfraMonitoringK8s/utils.tsx:11
- Draft comment:
TheorderBy
field uses 'cpu' which does not match any field inK8sPodsData
. Ensure the column name is correct. - Reason this comment was not posted:
Comment did not seem useful.
5. frontend/src/container/InfraMonitoringK8s/Nodes/K8sNodesList.tsx:112
- Draft comment:
Avoid using inline styles in React components. Instead, use external stylesheets, CSS classes, or styled components. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_vV8t4Cw8NM6YDObK
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
</div> | ||
), | ||
clusterName: node.meta.k8s_cluster_name, | ||
cpuUtilization: node.nodeCPUUsage === -1 ? '-' : node.nodeCPUUsage, |
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.
Showing - instead of -1 to denote invalid value
8a1558a
to
c951cd0
Compare
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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! Incremental review on 26a6959 in 45 seconds
More details
- Looked at
42
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringK8s/Nodes/K8sNodesList.tsx:404
- Draft comment:
Consider creating a utility function to handle resettingexpandedRowKeys
to improve code readability and maintainability. This is applicable in multiple places wheresetExpandedRowKeys([])
is used. - Reason this comment was not posted:
Confidence changes required:50%
ThesetExpandedRowKeys
is used in multiple places to reset the expanded rows. It would be more efficient to use a single function to handle this logic, improving code readability and maintainability.
2. frontend/src/container/InfraMonitoringK8s/Nodes/K8sNodesList.tsx:478
- Draft comment:
Consider creating a utility function to handle resettingexpandedRowKeys
to improve code readability and maintainability. This is applicable in multiple places wheresetExpandedRowKeys([])
is used. - Reason this comment was not posted:
Confidence changes required:50%
TheexpandedRowKeys
state is being reset in multiple places. It would be beneficial to have a utility function to handle this logic, which would improve code readability and maintainability.
3. frontend/src/container/InfraMonitoringK8s/Nodes/K8sNodesList.tsx:480
- Draft comment:
Avoid using inline styles for theSpin
component. Consider using CSS classes or styled components instead. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_vYKGCHCTPSGUmUMa
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
26a6959
to
b5d57e1
Compare
b5d57e1
to
ba944d0
Compare
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
1 similar comment
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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! Incremental review on 1b0d029 in 52 seconds
More details
- Looked at
191
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. frontend/src/api/infra/getHostAttributeKeys.ts:24
- Draft comment:
Consider making thelimit
parameter configurable instead of hardcoding it to 500. This will provide more flexibility and potentially improve performance for different use cases. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The limit of 500 seems like a reasonable default that would work for most autocomplete scenarios. Making it configurable would add complexity without clear benefits. There's no evidence that different use cases need different limits. The comment is speculative about "potentially improve performance" without concrete justification.
Maybe there are specific use cases where a different limit would be beneficial. The hardcoded value could become a maintenance issue if requirements change.
Without concrete evidence of different limit requirements or performance issues, adding configuration complexity isn't justified. If needs change, the value can be updated then.
Delete the comment as it suggests adding complexity without clear evidence of benefit. The hardcoded 500 limit is reasonable for autocomplete.
2. frontend/src/container/InfraMonitoringK8s/Nodes/K8sNodesList.tsx:317
- Draft comment:
TheshowHeader
property is set tofalse
, which hides the table headers. Ensure this is the intended behavior, as headers are typically important for data tables. - Reason this comment was not posted:
Confidence changes required:50%
InK8sNodesList.tsx
, theshowHeader
property of theTable
component is set tofalse
. This might not be the intended behavior as it hides the table headers, which are usually important for understanding the data being displayed.
3. frontend/src/container/InfraMonitoringK8s/Nodes/K8sNodesList.tsx:317
- Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This is also applicable to other inline styles in this file. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_H93uRSdOk7nJ9bvj
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
186f768
to
6d441f3
Compare
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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! Incremental review on 186f768 in 1 minute and 34 seconds
More details
- Looked at
339
lines of code in13
files - Skipped
0
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringK8s/Nodes/K8sNodesList.styles.scss:8
- Draft comment:
Avoid using!important
in CSS unless absolutely necessary. It makes the code harder to maintain and override. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The use of !important here seems deliberate to ensure consistent table column widths, likely fighting against Ant Design's default table styles. While generally avoiding !important is good practice, in cases of overriding third-party component styles it's often necessary. Without seeing the actual rendering and style conflicts, we can't be certain if there's a better alternative.
I might be too lenient on accepting !important when dealing with third-party components. There could be alternative approaches using more specific selectors or component customization.
While alternatives might exist, the comment doesn't provide specific actionable solutions, and !important is a common necessity when working with third-party UI libraries like Ant Design.
Delete the comment as it's not providing clear actionable feedback, and the use of !important appears intentional for overriding third-party component styles.
2. frontend/src/container/InfraMonitoringK8s/Nodes/NodeDetails/Events/NodeEvents.tsx:80
- Draft comment:
Avoid usingas any
for type casting. Ensurefilters
is correctly typed and initialized. - Reason this comment was not posted:
Comment did not seem useful.
3. frontend/src/container/InfraMonitoringK8s/Nodes/NodeDetails/Logs/NodeLogsDetailedView.tsx:53
- Draft comment:
Avoid usingas any
for type casting. Ensurefilters
is correctly typed and initialized. - Reason this comment was not posted:
Marked as duplicate.
4. frontend/src/container/InfraMonitoringK8s/Nodes/NodeDetails/Traces/NodeTraces.tsx:70
- Draft comment:
Avoid usingas any
for type casting. Ensurefilters
is correctly typed and initialized. - Reason this comment was not posted:
Marked as duplicate.
5. frontend/src/container/InfraMonitoringK8s/Pods/PodDetails/Events/Events.tsx:80
- Draft comment:
Avoid usingas any
for type casting. Ensurefilters
is correctly typed and initialized. - Reason this comment was not posted:
Marked as duplicate.
6. frontend/src/container/InfraMonitoringK8s/Pods/PodDetails/PodLogs/PodLogsDetailedView.tsx:53
- Draft comment:
Avoid usingas any
for type casting. Ensurefilters
is correctly typed and initialized. - Reason this comment was not posted:
Marked as duplicate.
7. frontend/src/container/InfraMonitoringK8s/Pods/PodDetails/PodTraces/PodTraces.tsx:70
- Draft comment:
Avoid usingas any
for type casting. Ensurefilters
is correctly typed and initialized. - Reason this comment was not posted:
Marked as duplicate.
8. frontend/src/container/InfraMonitoringK8s/Nodes/NodeDetails/NodeDetails.tsx:95
- Draft comment:
Avoid using inline styles. Use external stylesheets, CSS classes, or styled components instead. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_8ZdfvAp2H7ursDOl
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
❌ Changes requested. Incremental review on 9a57f08 in 1 minute and 11 seconds
More details
- Looked at
239
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringK8s/commonUtils.tsx:108
- Draft comment:
TheEntityProgressBar
usesgetStrokeColorForLimitUtilization
, which might not be appropriate for request utilization. Consider usinggetStrokeColorForRequestUtilization
for request progress bars to maintain consistent visual representation. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
Without more context about how EntityProgressBar is used, we can't know if it's meant for requests or limits. The name is generic and could be used for either. The comment is speculative ("might not be appropriate") rather than definitive. We don't have enough evidence to know if this is actually incorrect.
The comment could be correct - if EntityProgressBar is used for requests, using the limit colors would be wrong. We're missing the usage context.
Per the rules, we should delete speculative comments and comments that require more context to validate. The comment uses "might" language and we can't verify the correct behavior.
Delete the comment since it's speculative and we lack the context to verify if it's actually incorrect.
Workflow ID: wflow_WS9bXdpQVWDvcAHX
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
className="progress-bar" | ||
showInfo={false} | ||
/> | ||
<Typography.Text style={{ fontSize: '10px' }}>{percentage}%</Typography.Text> |
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.
Avoid using inline styles. Consider using CSS classes or styled components instead.
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
❌ Changes requested. Incremental review on dba5695 in 1 minute and 25 seconds
More details
- Looked at
273
lines of code in10
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_AcwUgzo1SEUYbQTf
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
}, | ||
], | ||
}, | ||
}), | ||
[currentQuery], | ||
); | ||
|
||
console.log({ updatedCurrentQuery }); |
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.
Remove console.log
statement to avoid cluttering the console in production.
Summary
Implement the nodes list table in Infra Monitoring
Related Issues / PR's
N/A
Screenshots
N/A
Affected Areas and Manually Tested Areas
Infra Monitoring section
Important
Implement Kubernetes nodes list feature in Infra Monitoring with new API, UI components, and hooks for data fetching and display.
getK8sNodesList
ingetK8sNodesList.ts
to fetch Kubernetes nodes.K8sNodesListPayload
,K8sNodesData
, andK8sNodesListResponse
.K8sNodesList
component inK8sNodesList.tsx
for displaying nodes with pagination and sorting.InfraMonitoringK8s.tsx
to includeK8sNodesList
.K8sFiltersSidePanel.tsx
andK8sHeader.tsx
for dynamic column management.useGetK8sNodesList
inuseGetK8sNodesList.ts
for querying nodes list.utils.tsx
for data formatting and table columns.constants.ts
to include nodes-related quick filters.This description was created by for dba5695. It will automatically update as commits are pushed.