-
Notifications
You must be signed in to change notification settings - Fork 28
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
CNV-29738: Adding netwrok ping tool -checkups #1587
Conversation
@metalice: This pull request references CNV-29738 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.15.0" version, but it targets "CNV v4.15.0" instead. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
src/views/checkups/network/components/CheckupsNetworkStatusIcon.tsx
Outdated
Show resolved
Hide resolved
src/views/checkups/network/components/form/CheckupsNetworkFormNads.tsx
Outdated
Show resolved
Hide resolved
src/views/checkups/network/components/form/CheckupsNetworkFormNodes.tsx
Outdated
Show resolved
Hide resolved
const [roles, loadingRoles] = useK8sWatchResource<IoK8sApiRbacV1ClusterRole[]>({ | ||
groupVersionKind: modelToGroupVersionKind(ClusterRoleModel), | ||
isList: true, | ||
}); | ||
|
||
const [roleBinding, loadingRolesBinding] = useK8sWatchResource< | ||
IoK8sApiRbacV1ClusterRoleBinding[] | ||
>({ | ||
groupVersionKind: modelToGroupVersionKind(ClusterRoleBindingModel), | ||
isList: true, | ||
}); | ||
const isLatencyRole = useMemo( | ||
() => findObjectByName(roles, KUBEVIRT_VM_LATENCY_CHECKER), | ||
[roles], | ||
); | ||
|
||
const isConfigMapRole = useMemo( | ||
() => findObjectByName(roles, KIAGNOSE_CONFIGMAP_ACCESS), | ||
[roles], | ||
); | ||
|
||
const isLatencyRoleBinding = useMemo( | ||
() => findObjectByName(roleBinding, KUBEVIRT_VM_LATENCY_CHECKER), | ||
[roleBinding], | ||
); | ||
|
||
const isConfigMapRoleBinding = useMemo( | ||
() => findObjectByName(roleBinding, KIAGNOSE_CONFIGMAP_ACCESS), | ||
[roleBinding], | ||
); | ||
|
||
const isServiceAccount = useMemo( | ||
() => findObjectByName(serviceAccounts, VM_LATENCY_CHECKUP_SA), | ||
[serviceAccounts], | ||
); |
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.
Do we know the namespace of those resources? default
or maybe the current namespace
?
In that case, we could watch directly the resource and see if the error code is 404
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.
its the active/current namespace, for serviceAccounts , the first call, as I used there, the other resources I did tried using the namespace but no luck :(
I can try again to be sure
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.
This looks great! I just have a few comments/suggestions.
|
||
import { deleteNetworkCheckup, rerunNetworkCheckup, STATUS_SUCCEEDED } from '../utils/utils'; | ||
|
||
const CheckupsNetworkActions = ({ |
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.
I suggest renaming things from CheckupsNetwork...
to NetworkCheckups...
Checkups
is a noun and Network
is an adjective and the adjective should precede the noun. I know this isn't a small change, but it would make it more readable and consistent. NetworkCheckup...
is used in some places while CheckupsNetwork...
is used in others.
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.
im sure you're right about the English :)
I choose this way because of routing, as checkups are first, then network, then form, etc...
if there is something that is backwards I think its better change it like u mention some are netwrokcheckups
wdyt?
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.
I don't think it's necessary to constrain component/file names to match the routing path since the way we approach each of those is different. These file names just read wrong to my brain.
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.
I understand what you are saying, and u have a point, but currently, I can see this is how we are using it in many places. It's in our guidelines (need to find the link) that is written it should be like that, VM list, vmi list, and templates are using this convention, so I'm guessing it is okay here
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.
I believe you, but don't remember that discussion. Let me know if you find the link.
src/views/checkups/network/components/CheckupsNetworkStatusIcon.tsx
Outdated
Show resolved
Hide resolved
src/views/checkups/network/components/form/CheckupsNetworkFormNads.tsx
Outdated
Show resolved
Hide resolved
src/views/checkups/network/components/form/CheckupsNetworkFormNodes.tsx
Outdated
Show resolved
Hide resolved
src/views/checkups/network/list/CheckupsNetworkListEmptyState.tsx
Outdated
Show resolved
Hide resolved
src/views/checkups/network/list/CheckupsNetworkListEmptyState.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Matan Schatzman <mschatzm@redhat.com>
Signed-off-by: Matan Schatzman <mschatzm@redhat.com>
@pcbailey @upalatucci ptal |
lgtm for me. @pcbailey it's on you |
Signed-off-by: Matan Schatzman <mschatzm@redhat.com>
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: metalice, pcbailey The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
📝 Description
Adding network checkups tool
https://docs.google.com/document/d/15xX-JPNt90bBGEVWhqKe6o-AyeluIVc6519hzXUF0_E/edit
https://issues.redhat.com/browse/CNV-29738
🎥 Demo