Add health check page #5026
Add health check page #5026
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
openshift-ci-robot
added
the
component/core
vikram-raj
force-pushed
the
pull-4931
branch
3 times, most recently
from
101e0c2
to
d0efd1a
Compare
|
/kind feature |
openshift-ci-robot
added
the
kind/feature
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
/hold Depends on PR #5045 |
openshift-ci-robot
added
the
do-not-merge/hold
vikram-raj
force-pushed
the
pull-4931
branch
2 times, most recently
from
43bff32
to
b5b8597
Compare
frontend/packages/dev-console/src/components/health-checks/AddHealthCheck.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/dev-console/src/components/health-checks/AddHealthCheckPage.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/dev-console/src/components/health-checks/AddHealthCheck.tsx
Outdated
Show resolved
Hide resolved
vikram-raj
force-pushed
the
pull-4931
branch
4 times, most recently
from
fe4c46c
to
1892c05
Compare
openshift-ci-robot
added
the
needs-rebase
|
/assign @christianvogt |
There was a problem hiding this comment.
@vikram-raj great job getting this feature in! Approved by UX
| ); | ||
|
|
||
| const handleAlertAction = (name: string, namespace: string) => { | ||
| const hideHealthCheckAlert = [...hideHealthCheckAlertFor, `${namespace}/${name}`]; |
There was a problem hiding this comment.
${namespace}/${name} is not a unique identifier because I can have a two different workload types which have the same name.
You are better off using the UID instead so that if someone deletes a workload and then recreates it with the same name, they are prompted again.
| }; | ||
|
|
||
| const showAlert = | ||
| _.includes(addHealthChecksRefs, referenceForModel(model)) && |
There was a problem hiding this comment.
This will always be true because you return null on line 39.
| isInline | ||
| > | ||
| {_.size(containersName) > 1 ? 'Containers' : 'Container'}{' '} | ||
| {_.map(containersName).join(', ')} does not have health checks to ensure your |
There was a problem hiding this comment.
does becomes do when plural.
Furthermore I see this in the design:
| <div> | ||
| Managing health check for:{' '} | ||
| <ContainerDropdown | ||
| currentKey={currentKey} | ||
| containers={containers} | ||
| onChange={handleSelectContainer} | ||
| /> |
There was a problem hiding this comment.
Missing link back to workload and learn more:
Also notice that for a single container there is no dropdown. Only when there are multiple containers should there be a dropdown:
| handleReset={handleReset} | ||
| errorMessage={status && status?.errors?.json?.message} | ||
| isSubmitting={isSubmitting} | ||
| submitLabel="Save" |
There was a problem hiding this comment.
Label should be Add if there are no health check according to UX.
| <title>Health Checks</title> | ||
| </Helmet> | ||
| <div> | ||
| <PageHeading title="Health Checks" /> |
There was a problem hiding this comment.
When adding the page should be named Add Health Checks and when editing it is Edit Health Checks
| const { ns, kind, name, containerName } = match.params; | ||
| const resource: FirehoseResource[] = [ | ||
| { | ||
| kind: kind === KnativeServiceModel.kind ? referenceForModel(KnativeServiceModel) : kind, |
There was a problem hiding this comment.
You should be using a qualified reference for knative always so that you don't need to do these one off checks.
| return updatedResource; | ||
| }; | ||
|
|
||
| const AddHealthCheckWrapper: React.FC<AddHealthCheckWrapperProps> = ({ |
There was a problem hiding this comment.
Calling things a Wrapper as a first class component is a poor name.
Perhaps simply HealthChecksForm.
| model.kind}/${resource.metadata.name}/containers/${containersName[0]}/health-checks`; | ||
|
|
||
| return ( | ||
| <> |
There was a problem hiding this comment.
fragment is not needed here.
There was a problem hiding this comment.
I think we need this fragment here. Because I conditionally rendering the alert.
There was a problem hiding this comment.
return { showAlert ? <...> : null;
| currentContainer: string; | ||
| }; | ||
|
|
||
| const updateHealthChecksProbe = ( |
There was a problem hiding this comment.
move to a separate util for ease of testing.
| const container = _.filter( | ||
| resource.data.spec.template.spec.containers, | ||
| (data) => data.name === currentContainer, | ||
| ); |
There was a problem hiding this comment.
Use find instead so that you don't need to do container[0] always.
| import HealthChecks from './HealthChecks'; | ||
|
|
||
| type AddHelathProps = { | ||
| resource?: FirehoseResult<K8sResourceKind>; |
There was a problem hiding this comment.
You don't need FirehoseResult prop here. Just use K8sResourceKind.
You already check in the parent if the data is loaded and from an API point of view, this form should only need to deal with the resource itself and not the firehose payload as well.
| ); | ||
| }; | ||
|
|
||
| const containers = resource.loaded && _.keyBy(data.spec.template.spec.containers, 'name'); |
There was a problem hiding this comment.
resource is always loaded according to the current flow. Seem comment about props.
| isList: false, | ||
| name, | ||
| prop: 'resource', | ||
| optional: true, |
There was a problem hiding this comment.
Should this really be optional? I think we want to see errors in this case.
| accessReview: { | ||
| group: model.apiGroup, | ||
| resource: model.plural, | ||
| name: obj.metadata.name, | ||
| namespace: obj.metadata.namespace, | ||
| verb: 'update', | ||
| }, |
There was a problem hiding this comment.
Is there no way for someone without access to SEE the health checks?
@serenamarie125 what's the expectation for users with view access but not edit. Can they get to the forms in a read only mode?
| loader: async () => | ||
| ( | ||
| await import( | ||
| './components/health-checks/AddHealthCheckPage' /* webpackChunkName: "dev-console-healthCheck" */ |
There was a problem hiding this comment.
nit: I think AddHealthCheckPage should simply be HealthChecksPage since it is used for edit and add (maybe even for view too later)
| resource: K8sResourceKind; | ||
| model: K8sKind; |
There was a problem hiding this comment.
We already have the resource. Why do we need the model as well?
Resource should be sufficient.
There was a problem hiding this comment.
We need it for getting the referenceForModel for resource like knative.
There was a problem hiding this comment.
You can use referenceFor the resource itself has all the information.
|
@serenamarie125 I'm not sure what the expected UX is when wanting to edit the health checks for multiple containers. But right now you can only do one container at a time. You must save in between editing each container. Worst part is that once you edit the health checks for one container, you're thrown back to the previous page and need to enter the form again. |
| <Button | ||
| variant="link" | ||
| component="a" | ||
| href="https://docs.openshift.com/container-platform/3.11/dev_guide/application_health.html" |
There was a problem hiding this comment.
I really don't think we want to be pointing to old documentation. Not sure what UX had in mind. Make an issue to get an updated link before release.
There was a problem hiding this comment.
When I made the first comment about matching UX design, I wasn't sure if they provided you with a proper link. If we don't have one, we can also omit this link for now until we know what to put. Either way we need an issue to track updating this URL.
There was a problem hiding this comment.
Created an issue to update the doc link when it is available for 4.5. https://issues.redhat.com/browse/ODC-3620
| href: `/k8s/ns/${obj.metadata.namespace}/${obj.kind || model.kind}/${ | ||
| obj.metadata.name | ||
| }/containers/${obj.spec.template.spec.containers[0].name}/health-checks`, |
There was a problem hiding this comment.
You cannot use kind here.
eg. my knative service details page:
http://localhost:9000/k8s/ns/cvogt/serving.knative.dev~v1~Service/react-web-app
my health checks page for the same knative service:
http://localhost:9000/k8s/ns/cvogt/Service/react-web-app/containers/user-container/health-checks
expected to be:
http://localhost:9000/k8s/ns/cvogt/serving.knative.dev~v1~Service/react-web-app/containers/user-container/health-checks
need to use referenceFor(obj)
| const addHealthChecksLink = `/k8s/ns/${resource.metadata.namespace}/${resource.kind || | ||
| model.kind}/${resource.metadata.name}/containers/${containersName[0]}/health-checks`; |
There was a problem hiding this comment.
Link should not rely on kind alone. See other comment.
| history.push( | ||
| `/k8s/ns/${resource.metadata.namespace}/${resource.kind}/${resource.metadata.name}/containers/${containerName}/health-checks`, |
There was a problem hiding this comment.
URL cannot rely on kind alone.
I think you should replace the URL instead of push. That way on submit / reset you can go back in history.
Currently switching containers and pressing cancel gives an odd experience.
There was a problem hiding this comment.
replaced the url in history instead of push.
| .then(() => { | ||
| actions.setSubmitting(false); | ||
| actions.setStatus({ error: '' }); | ||
| history.push(`/topology/ns/${updatedResource.metadata.namespace}`); |
There was a problem hiding this comment.
We shouldn't be going back to topology always. We should return to the page the user was at before.
Keep in mind that in between the user may have changed containers so back in history doesn't work. Unless the dropdown just replaced the URL; i think it should replace instead of push.
There was a problem hiding this comment.
Fixed it. Instead of redirecting it to topology every time. now redirecting it back from where the user is landing to the health checks page.
| const { ns, kind, name, containerName } = match.params; | ||
| const resource: FirehoseResource[] = [ | ||
| { | ||
| kind: kind === KnativeServiceModel.kind ? referenceForModel(KnativeServiceModel) : kind, |
There was a problem hiding this comment.
If your URL was properly qualified you'd have the correct kind. Right now your kind is just Service which is the wrong Service.
| kind: kind === KnativeServiceModel.kind ? referenceForModel(KnativeServiceModel) : kind, | |
| kind: referenceForModel(kind), |
You can tell because the link to the workload for a knative service is this:
Which actually brings you to the wrong page.
| variant="default" | ||
| title="Health Checks" | ||
| action={ | ||
| <AlertActionCloseButton onClose={() => handleAlertAction(resource.metadata.uid)} /> |
There was a problem hiding this comment.
You really don't need to pass a param to handleAlertAction because it has access to resource.metadata.uid just the same.
| <AlertActionCloseButton onClose={() => handleAlertAction(resource.metadata.uid)} /> | |
| <AlertActionCloseButton onClose={handleAlertAction} /> |
There was a problem hiding this comment.
yes, now directly using the resource.metadata.uid
|
/approve |
openshift-ci-robot
added
the
approved
|
/retest |
| !healthCheckAdded && !_.includes(hideHealthCheckAlertFor, resource.metadata.uid); | ||
|
|
||
| const addHealthChecksLink = `/k8s/ns/${resource.metadata.namespace}/${ | ||
| resource.kind === KnativeServiceModel.kind ? referenceFor(resource) : resource.kind |
There was a problem hiding this comment.
You should not be doing any special conditions for knative.
You need to get the model for resource and check if it has crd === true.
There are utility functions in the resource-link file.
So... you may need to pass model afterall or use the resource info to fetch it. Thought there was a util that does it all but maybe not. There is connectToModel but you may as well pass in the model if you already have it.
| return { | ||
| label: 'Edit Health Checks', | ||
| href: `/k8s/ns/${obj.metadata.namespace}/${ | ||
| model.kind === KnativeServiceModel.kind ? referenceForModel(KnativeServiceModel) : model.kind |
There was a problem hiding this comment.
Same comment regarding checking knative.
| setFieldValue('containerName', containerName); | ||
| setFieldValue('healthChecks', getHealthChecksData(resource, containerIndex)); | ||
| history.replace( | ||
| `/k8s/ns/${resource.metadata.namespace}/${resource.kind}/${resource.metadata.name}/containers/${containerName}/health-checks`, |
There was a problem hiding this comment.
This url is won't work for knative services.
See other comments regarding URLs.
|
/lgtm @vikram-raj please raise issues to address remaining comments or fix and get lgtm again |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: christianvogt, invincibleJai, rohitkrai03, serenamarie125, vikram-raj 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 |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
I created issues and will raise another PR |
Fixes:
Story - https://issues.redhat.com/browse/ODC-3331 and https://issues.redhat.com/browse/ODC-3466
Analysis / Root cause:
As a user, I do not ensure that my application is running correctly. And there is not any with the application.
Solution Description:
Add an option to add health to the application. This PR adds an option to add health check the context menu and action menu of the application and a new page for the add/edit the health check.
Screen shots / Gifs for design review:
Tests
Browser conformance: