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

debsmita1 · 2021-06-14T19:33:03Z

Fixes:
https://issues.redhat.com/browse/ODC-6015

Root analysis:
The field input type was changed from number to text to bypass the browser validation (#8198), because of this the probe data is received as a string instead of number

Solution description:

The field values received is converted to a number before updating the resource
Fixed the Form footer alignment

GIF:

debsmita1 · 2021-06-14T19:34:30Z

/kind bug

jerolimov

Tested this locally, works fine. But please let us use toInteger instead of toNumber.

frontend/packages/dev-console/src/components/health-checks/create-health-checks-probe-utils.ts

jerolimov · 2021-06-15T06:55:36Z

@debsmita1 Can you also update the existing test in create-health-check-probe-utils.spec.ts? Maybe its enough to convert the numbers at the top of this file to strings?

debsmita1 · 2021-06-15T09:55:34Z

@debsmita1 Can you also update the existing test in create-health-check-probe-utils.spec.ts? Maybe its enough to convert the numbers at the top of this file to strings?

Done

andrewballantyne · 2021-06-15T13:49:34Z

frontend/packages/dev-console/src/components/health-checks/AddHealthChecks.tsx

+          <p>
+            {t('devconsole~Health checks for')} &nbsp;
+            <ResourceLink
+              kind={referenceFor(resource)}
+              name={name}
+              namespace={namespace}
+              title={name}
+              inline
+            />
+          </p>


Nit: This should probably be using <Trans>...</Trans>. Translators should always be given full context as not all languages keep the same sentence structure. Sometimes the noun is first, sometimes it's last.

andrewballantyne · 2021-06-15T13:56:49Z

frontend/packages/dev-console/src/components/health-checks/health-checks-types.ts

+  initialDelaySeconds: number | string;
+  periodSeconds: number | string;
+  timeoutSeconds: number | string;
+  successThreshold: number | string;


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.

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

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.

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?

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.

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.

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).

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

nemesis09 · 2021-06-15T18:05:05Z

frontend/packages/dev-console/src/components/health-checks/create-health-checks-probe-utils.ts

@@ -6,8 +6,8 @@ import { healthChecksDefaultValues } from './health-checks-probe-utils';

 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) }),


could we use parseInt here instead of lodash?

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

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

@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.

jerolimov · 2021-06-15T19:59:51Z

/retest

andrewballantyne

Please log a backlog ticket with my two comments and complete it this sprint (separate of this PR).

jerolimov

Verified import flow with an health check and adding/modifing a health check later on an existing deployment.

/lgtm

jerolimov · 2021-06-16T09:11:27Z

/retest

openshift-ci · 2021-06-16T09:12:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, debsmita1, jerolimov

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:

~~frontend/packages/dev-console/OWNERS~~ [andrewballantyne,jerolimov]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

debsmita1 · 2021-06-16T13:43:02Z

/cherry-pick release-4.8

openshift-cherrypick-robot · 2021-06-16T13:43:39Z

@debsmita1: #9237 failed to apply on top of branch "release-4.8":

Applying: convert probe data values from string to number type
Using index info to reconstruct a base tree...
M	frontend/packages/dev-console/src/components/health-checks/AddHealthChecks.tsx
M	frontend/packages/dev-console/src/components/health-checks/__tests__/create-health-checks-probe-data.ts
M	frontend/packages/dev-console/src/components/health-checks/create-health-checks-probe-utils.ts
Falling back to patching base and 3-way merge...
Auto-merging frontend/packages/dev-console/src/components/health-checks/create-health-checks-probe-utils.ts
Auto-merging frontend/packages/dev-console/src/components/health-checks/__tests__/create-health-checks-probe-data.ts
Auto-merging frontend/packages/dev-console/src/components/health-checks/AddHealthChecks.tsx
CONFLICT (content): Merge conflict in frontend/packages/dev-console/src/components/health-checks/AddHealthChecks.tsx
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 convert probe data values from string to number type
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.8

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.

openshift-ci · 2021-06-16T15:28:25Z

@debsmita1: All pull requests linked via external trackers have merged:

openshift/console#9237

Bugzilla bug 1972777 has been moved to the MODIFIED state.

In response to this:

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

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.

debsmita1 · 2021-06-16T15:32:55Z

/bugzilla refresh

openshift-ci · 2021-06-16T15:32:57Z

@debsmita1: Bugzilla bug 1972777 is in an unrecognized state (MODIFIED) and will not be moved to the MODIFIED state.

In response to this:

/bugzilla refresh

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.

debsmita1 · 2021-06-16T15:37:12Z

/bugzilla refresh

openshift-ci · 2021-06-16T15:37:16Z

@debsmita1: Bugzilla bug 1972777 is in an unrecognized state (MODIFIED) and will not be moved to the MODIFIED state.

In response to this:

/bugzilla refresh

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.

openshift-ci bot requested review from andrewballantyne and jerolimov June 14, 2021 19:33

openshift-ci bot added the component/dev-console Related to dev-console label Jun 14, 2021

openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Jun 14, 2021

jerolimov suggested changes Jun 15, 2021

View reviewed changes

frontend/packages/dev-console/src/components/health-checks/create-health-checks-probe-utils.ts Outdated Show resolved Hide resolved

frontend/packages/dev-console/src/components/health-checks/create-health-checks-probe-utils.ts Outdated Show resolved Hide resolved

debsmita1 force-pushed the fix-probedata branch 2 times, most recently from 7a810ca to 0b3a464 Compare June 15, 2021 09:54

debsmita1 requested a review from jerolimov June 15, 2021 09:55

andrewballantyne suggested changes Jun 15, 2021

View reviewed changes

nemesis09 reviewed Jun 15, 2021

View reviewed changes

andrewballantyne approved these changes Jun 15, 2021

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 15, 2021

debsmita1 force-pushed the fix-probedata branch from 0b3a464 to 5ccbe27 Compare June 16, 2021 05:09

openshift-ci bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated labels Jun 16, 2021

debsmita1 force-pushed the fix-probedata branch from 5ccbe27 to b805444 Compare June 16, 2021 07:22

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2021

convert probe data values from string to number type

2d52e05

debsmita1 force-pushed the fix-probedata branch from b805444 to 2d52e05 Compare June 16, 2021 07:36

jerolimov approved these changes Jun 16, 2021

View reviewed changes

openshift-ci bot assigned jerolimov Jun 16, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2021

openshift-merge-robot merged commit 46ee077 into openshift:master Jun 16, 2021

debsmita1 changed the title ~~Unable to edit the default Health check probe values~~ Bug 1972777: Unable to edit the default Health check probe values Jun 16, 2021

debsmita1 mentioned this pull request Jun 16, 2021

[release-4.8] Bug 1972788: Convert probe data values from string to number type #9277

Merged

spadgett added this to the v4.9 milestone Jul 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

debsmita1 commented Jun 14, 2021 •

edited

debsmita1 commented Jun 14, 2021

jerolimov left a comment

jerolimov commented Jun 15, 2021

debsmita1 commented Jun 15, 2021

andrewballantyne Jun 15, 2021

andrewballantyne Jun 15, 2021

debsmita1 Jun 15, 2021

andrewballantyne Jun 15, 2021

jerolimov Jun 15, 2021

andrewballantyne Jun 15, 2021

jerolimov Jun 15, 2021

andrewballantyne Jun 15, 2021

debsmita1 Jun 16, 2021

nemesis09 Jun 15, 2021

jerolimov Jun 15, 2021

nemesis09 Jun 16, 2021 •

edited

andrewballantyne Jun 16, 2021 •

edited

jerolimov commented Jun 15, 2021

andrewballantyne left a comment

jerolimov left a comment

jerolimov commented Jun 16, 2021

openshift-ci bot commented Jun 16, 2021

debsmita1 commented Jun 16, 2021

openshift-cherrypick-robot commented Jun 16, 2021

openshift-ci bot commented Jun 16, 2021

debsmita1 commented Jun 16, 2021

openshift-ci bot commented Jun 16, 2021

debsmita1 commented Jun 16, 2021

openshift-ci bot commented Jun 16, 2021

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

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

Conversation

debsmita1 commented Jun 14, 2021 • edited

debsmita1 commented Jun 14, 2021

jerolimov left a comment

Choose a reason for hiding this comment

jerolimov commented Jun 15, 2021

debsmita1 commented Jun 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesis09 Jun 16, 2021 • edited

Choose a reason for hiding this comment

andrewballantyne Jun 16, 2021 • edited

Choose a reason for hiding this comment

jerolimov commented Jun 15, 2021

andrewballantyne left a comment

Choose a reason for hiding this comment

jerolimov left a comment

Choose a reason for hiding this comment

jerolimov commented Jun 16, 2021

openshift-ci bot commented Jun 16, 2021

debsmita1 commented Jun 16, 2021

openshift-cherrypick-robot commented Jun 16, 2021

openshift-ci bot commented Jun 16, 2021

debsmita1 commented Jun 16, 2021

openshift-ci bot commented Jun 16, 2021

debsmita1 commented Jun 16, 2021

openshift-ci bot commented Jun 16, 2021

debsmita1 commented Jun 14, 2021 •

edited

nemesis09 Jun 16, 2021 •

edited

andrewballantyne Jun 16, 2021 •

edited