Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Bug 1972777: Unable to edit the default Health check probe values #9237

Merged
merged 1 commit into from
Jun 16, 2021
Merged

Bug 1972777: Unable to edit the default Health check probe values #9237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion frontend/packages/dev-console/locales/en/devconsole.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@
"Edit health checks": "Edit health checks",
"Add health checks": "Add health checks",
"Learn more": "Learn more",
"Health checks for": "Health checks for",
"Health checks for &nbsp;<1></1>": "Health checks for &nbsp;<1></1>",
"Container": "Container",
"Container not found": "Container not found",
"Readiness probe": "Readiness probe",
Expand Down
66 changes: 35 additions & 31 deletions frontend/packages/dev-console/src/components/health-checks/AddHealthChecks.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@ import { ExternalLinkAltIcon } from '@patternfly/react-icons';
import { FormikProps, FormikValues } from 'formik';
import * as _ from 'lodash';
import Helmet from 'react-helmet';
import { useTranslation } from 'react-i18next';
import { useTranslation, Trans } from 'react-i18next';
import {
ContainerDropdown,
history,
PageHeading,
ResourceLink,
ResourceIcon,
openshiftHelpBase,
} from '@console/internal/components/utils';
import { ContainerModel } from '@console/internal/models';
Expand Down Expand Up @@ -93,19 +92,21 @@ const AddHealthChecks: React.FC<FormikProps<FormikValues> & AddHealthChecksProps
</>
}
/>
<div className="odc-add-health-checks__body">
<p>
{t('devconsole~Health checks for')} &nbsp;
<ResourceLink
kind={referenceFor(resource)}
name={name}
namespace={namespace}
title={name}
inline
/>
</p>
<Form onSubmit={!viewOnly ? handleSubmit : undefined}>
<div>
<Form onSubmit={!viewOnly ? handleSubmit : undefined}>
<div className="odc-add-health-checks__body">
<p>
<Trans t={t} ns="devconsole">
Health checks for &nbsp;
<ResourceLink
kind={referenceFor(resource)}
name={name}
namespace={namespace}
title={name}
inline
/>
</Trans>
</p>
<p>
{t('devconsole~Container')} &nbsp;
{_.size(containers) > 1 ? (
<ContainerDropdown
Expand All @@ -114,24 +115,27 @@ const AddHealthChecks: React.FC<FormikProps<FormikValues> & AddHealthChecksProps
onChange={handleSelectContainer}
/>
) : (
<>
<ResourceIcon kind={ContainerModel.kind} />
{containers[0].name}
</>
<ResourceLink
kind={ContainerModel.kind}
name={containers[0].name}
linkTo={false}
inline
/>
)}
</div>
</p>
<br />
<HealthChecks resourceType={getResourcesType(resource)} />
<FormFooter
handleReset={handleReset}
errorMessage={status && status?.errors?.json?.message}
isSubmitting={isSubmitting}
submitLabel={healthCheckAdded ? t('devconsole~Save') : t('devconsole~Add')}
disableSubmit={isFormClean || !dirty || !_.isEmpty(errors) || isSubmitting}
resetLabel={t('devconsole~Cancel')}
hideSubmit={viewOnly}
/>
</Form>
</div>
</div>
<FormFooter
handleReset={handleReset}
errorMessage={status && status?.errors?.json?.message}
isSubmitting={isSubmitting}
submitLabel={healthCheckAdded ? t('devconsole~Save') : t('devconsole~Add')}
disableSubmit={isFormClean || !dirty || !_.isEmpty(errors) || isSubmitting}
resetLabel={t('devconsole~Cancel')}
hideSubmit={viewOnly}
/>
</Form>
</HealthCheckContext.Provider>
);
};
Expand Down
41 changes: 41 additions & 0 deletions ...ges/dev-console/src/components/health-checks/__tests__/create-health-checks-probe-data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,44 @@ export const enabledProbeData = {
timeoutSeconds: 1,
},
};

export const healthChecksFormInputData = {
healthChecks: {
readinessProbe: {
showForm: false,
enabled: true,
modified: false,
data: {
failureThreshold: '3',
requestType: RequestType.HTTPGET,
httpGet: {
scheme: 'HTTPS',
path: '/tmp/healthy',
port: 8080,
httpHeaders: [{ name: 'custom-header', value: 'value' }],
},
tcpSocket: {
port: 8080,
},
exec: { command: [''] },
initialDelaySeconds: '0',
periodSeconds: '10',
timeoutSeconds: '1',
successThreshold: '1',
},
},
livenessProbe: healthChecksDefaultValues,
startupProbe: {
showForm: false,
enabled: true,
modified: false,
data: {
...healthChecksDefaultValues.data,
requestType: RequestType.TCPSocket,
tcpSocket: {
port: 8081,
},
},
},
},
};
10 changes: 5 additions & 5 deletions ...end/packages/dev-console/src/components/health-checks/create-health-checks-probe-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ import { HealthCheckProbeData, RequestType, HealthChecksProbeType } from './heal

export const constructProbeData = (data: HealthCheckProbeData, resourceType?: Resources) => {
const probeData = {
...(data.failureThreshold && { failureThreshold: data.failureThreshold }),
...(data.successThreshold && { successThreshold: data.successThreshold }),
...(data.failureThreshold && { failureThreshold: _.toInteger(data.failureThreshold) }),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we use parseInt here instead of lodash?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some other _.toInteger calls in this function before. I think its good to use a similar way in one function.

Copy link
Contributor

@nemesis09 nemesis09 Jun 16, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think toInteger can be replaced using parseInt for all the cases.
shouldn't we prefer using standard built in functions to external library?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nemesis09 You're waaay too late to that party lol (I've been preaching that since Aug 2019 when I got hired lol). We have corrected it in a few places, but for the most part it is a mixed bag. There isn't too much loss to using lodash, and there is often a lot of code safety to gain in the utilities.

My preference is definitely to go with native functions when they do the same thing, to reduce our reliance on "magic" 3rd party utility functions. But in the end it doesn't really matter.

...(data.successThreshold && { successThreshold: _.toInteger(data.successThreshold) }),
...(data.requestType === RequestType.ContainerCommand && {
exec: data.exec,
}),
Expand All @@ -26,10 +26,10 @@ export const constructProbeData = (data: HealthCheckProbeData, resourceType?: Re
},
}),
...(data.initialDelaySeconds && {
initialDelaySeconds: data.initialDelaySeconds,
initialDelaySeconds: _.toInteger(data.initialDelaySeconds),
}),
...(data.periodSeconds && { periodSeconds: data.periodSeconds }),
...(data.timeoutSeconds && { timeoutSeconds: data.timeoutSeconds }),
...(data.periodSeconds && { periodSeconds: _.toInteger(data.periodSeconds) }),
...(data.timeoutSeconds && { timeoutSeconds: _.toInteger(data.timeoutSeconds) }),
};
return probeData;
};
Expand Down
12 changes: 5 additions & 7 deletions frontend/packages/dev-console/src/components/health-checks/health-checks-types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,8 @@ export enum RequestType {
ContainerCommand = 'command',
TCPSocket = 'tcpSocket',
}

export interface HealthCheckProbeData {
failureThreshold: number;
failureThreshold: number | string;
requestType?: string;
httpGet?: {
scheme: string;
Expand All @@ -25,12 +24,11 @@ export interface HealthCheckProbeData {
port: number;
};
exec?: { command?: string[] };
initialDelaySeconds: number;
periodSeconds: number;
timeoutSeconds: number;
successThreshold: number;
initialDelaySeconds: number | string;
periodSeconds: number | string;
timeoutSeconds: number | string;
successThreshold: number | string;
Comment on lines +27 to +30
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This muddiest the water of our TypeScript types. Either we need to convert at Input level to a number, and always treat it as number. Or we need to have two types... one for the formik data structure which are input value driven data. And then you want pure representation data that we talk about in "HealthCheck" terms.

Merging these two things together just allows us to not ever know by just looking at the type which state it is in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this for sake of adding a unit test where the input data is string, the utils are all strictly typed, and won't allow testing string data

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you need to mangle types for Tests, please extend the type in the test file and expand it there. Best not impact our application types for tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, maybe its a good idea to split this into two kinds of data.

HealthCheckProbe vs HealthCheckForm and then constructProbeData gets form data and return "kubernetes data".

But I don't know how big this change is. But it sounds not sooo bad, or?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are "just" inside a new release. This is not the time to concern ourselves with it. If this fix/bug is something we need to backport, then perhaps we go with what we have.

If it's not intended for backport or @debsmita1 you're okay with making a slimmed version for backport manually, we should 100% be looking towards the horizons when making type changes and not do something that lowers our quality and ease of use.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should backport this asap as this breaks the health checks, at least if the user changes one of this five numeric fields. So I hope that we pick this up for the first z-stream release.

I checked the code now again. Andrew is in general right that we should distinguish between k8s resource data and internal form data. From my understand the HealthChecksData is currently used as form data type, not as resource definition.

The convert from form data to resource data happen in updateHealthChecksProbe and getProbesData.

And the convert from resource data to form data in getHealthChecksData.

At the moment this last method doesn't convert the resource integers to form strings. But when the user changes the values in the form we currently save strings in the form data. So at the moment its right that we have number|string values in the form. (Status quo. Which doesn't mean that I liked this.)

So one option is that we create two clean types. One for the FormData and one for the ResourceData and update all methods above to ensure that they convert between strings and numbers. So that the type doesn't change anymore when the user touches a field.

The other one is to create a follow up issue (tech debt or bug) and accept that the form mixes two types at the moment. This makes at least the verification and backport of this fix easy. When I understand Andrew correctly this is also acceptable for him.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should backport this asap as this breaks the health checks, at least if the user changes one of this five numeric fields. So I hope that we pick this up for the first z-stream release.

No z-streams for 4.8 for at least 3 weeks, if not more. We need to go GA, then 1 or 2 z-streams (at roughly 1 week cadences) before we can get changes into it.

It's fine doing it a tech debt, but I am concerned about our approach to code. We have always had TypeScript lax feelings and this is a good case when we are opting for more lax types to multi-purpose them. I'm personally against this style of coding. We can take this as-is and port it back, wait 3ish weeks to merge it, and in the meantime work to clean this up in 4.9 asap (and it cannot be "on the backlog" imo).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logged ticket https://issues.redhat.com/browse/ODC-6037

}

export interface HealthCheckProbe {
showForm?: boolean;
enabled?: boolean;
Expand Down