[IA-4137] Add types to desired and existing runtime config #5079

jdcanas · 2024-09-12T18:39:51Z

No description provided.

msilva-broad · 2024-09-12T18:49:58Z

src/analysis/modal-utils.ts

+};
+
+// Auxiliary functions -- begin
+export const isGce = (runtimeType) => runtimeType === runtimeTypes.gceVm;


these three isXxxx functions could be ijmproved to be type predicates that return "is TypeX... But from our chat, I assume that will have to wait until we do a 2nd wave of type love.

yeah, since that will require nesting them in an additional field and thus functionally changing the modal itself, which I was able to avoid in this PR

msilva-broad · 2024-09-12T18:56:46Z

src/analysis/modal-utils.ts

+// extract desiredruntimeconfig into a type
+// TODO: converge type with getExistingEnvironmentConfig
+// TODO: type desired runtimetype
+export const buildDesiredEnvironmentConfig = (


if these util methods are only used by GcpComputeModal, maybe we can avoid adding them to the generic modal-utils file here and add them to a more specific helper file next to the only modal using it.

Yeah, I agree, this file needs a better name because these two methods (and the smaller helpers) are the main use case for it.

LizBaldo · 2024-09-13T13:32:33Z

src/analysis/modal-utils.ts

    autopauseThreshold: currentRuntimeDetails?.autopauseThreshold,
+    computeConfig,


Should these two be typed as well?

these are typed here https://github.com/DataBiosphere/terra-ui/pull/5079/files#diff-32a05189ab14623d039151b28dfee24908fc62cad51bf92afdb7767afbf7ef01R45

LizBaldo · 2024-09-13T13:33:59Z

src/analysis/modal-utils.ts

          ...(cloudService === cloudServices.GCE
            ? {
                zone: computeConfig.computeZone,
-                region: computeConfig.computeRegion,


So no need of the region anymore? Only the zone was used?

the region was duplicated on line 69 and 82 in both branches of the conditional, I simplified by moving it up to here https://github.com/DataBiosphere/terra-ui/pull/5079/files#diff-32a05189ab14623d039151b28dfee24908fc62cad51bf92afdb7767afbf7ef01R128

LizBaldo · 2024-09-13T13:41:48Z

src/analysis/modal-utils.ts

+  return {
+    hasGpu: computeConfig.hasGpu,
+    autopauseThreshold: computeConfig.autopauseThreshold,
+    computeConfig,


Same question here about typing these?

lucymcnatt

Left a couple questions + small thing I found when testing, the running compute cost is showing unknown specifically for the Standard VM compute type

lucymcnatt · 2024-09-16T14:42:55Z

src/analysis/modal-utils.ts

-          ...(currentRuntimeDetails?.timeoutInMinutes
-            ? {
-                timeoutInMinutes: currentRuntimeDetails?.timeoutInMinutes,
-              }


is the timeout not needed here anymore?

no, it is never used off the existing runtime config object

Instead of removing this, it'd be better to fix the bug where this isn't used, and loading a runtime with a custom timeout does not display the correct value - GcpComputeModal line 236 has const [timeoutInMinutes, setTimeoutInMinutes] = useState(null); - this could change to useState(currentRuntime?.timeoutInMinutes || null

or at least leaving a comment on that line with the needed change

this isn't actually a field on existing runtimes! I think the types demonstrate well that this is a field for desired runtime that we submit in the request, but it is not something leo returns back on existing.

I'll add a comment on the type

lucymcnatt · 2024-09-16T14:51:46Z

src/analysis/utils/gce-machines.ts

@@ -153,6 +153,8 @@ export const zonesToGpus = [
  { name: 'US-WEST4-C', validTypes: [] },
 ];

+// **Deprecated**
+// Use cloudServiceTypes in `runtime-config-models`


since this is deprecated, can we just ✂️ ?

Not without changing the logic... the TODO on line 135 of modal-utils is necessary before. Currently, we use this instead of the runtimeTypes in a lot of conditional logic. Hopefully if there is time for a second phase of this it can be deleted, but more testing is needed first.

mlilynolting

Are you able to add some tests of the new logic that builds the existing runtime config/the desired runtime config from the existing runtime config?

jdcanas · 2024-09-17T19:51:13Z

Are you able to add some tests of the new logic that builds the existing runtime config/the desired runtime config from the existing runtime config?

it took a lot of lines to test it.. def was a good idea

jdcanas · 2024-09-17T19:52:15Z

Left a couple questions + small thing I found when testing, the running compute cost is showing unknown specifically for the Standard VM compute type

Fixed the cost, thanks! The gpu type was garbled.

mlilynolting · 2024-09-18T14:01:36Z

Are you able to add some tests of the new logic that builds the existing runtime config/the desired runtime config from the existing runtime config?

it took a lot of lines to test it.. def was a good idea

thank you, the tests look great!

jdcanas · 2024-09-18T16:12:03Z

src/analysis/modals/ComputeModal/GcpComputeModal/GcpComputeModal.test.ts

+      const selectOption = await screen.findByText('2');
+      await user.click(selectOption);
+      const nextButton = await screen.findByText('Next');
+      await user.click(nextButton);


There was actually a bug that these tests relied on to pass that was fixed.

Essentially, the canUpdateRuntime function compares existing and desired configs and if it finds a difference say 'yes you can update'. The toolLabel in the desired config was incorrectly computed, and was undefined, which signaled to the modal an update was allowed (hence update button Enabled). The test on line 369 now correctly reads update should be disabled when no changes have been made, and this subsequent test actually requires a field to be changed (CPUs) for Update to be enabled.

before (note the undefined):

now (note two objects are now identical):

sonarcloud · 2024-09-19T17:31:22Z

Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
91.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

jdcanas added 2 commits September 11, 2024 12:17

add typing for existing runtime

147e687

changes in place to support desired and existing runtime typing

85418a6

jdcanas requested a review from a team as a code owner September 12, 2024 18:39

jdcanas requested review from LizBaldo, lucymcnatt, mlilynolting and msilva-broad September 12, 2024 18:42

msilva-broad reviewed Sep 12, 2024

View reviewed changes

LizBaldo reviewed Sep 13, 2024

View reviewed changes

jdcanas requested a review from LizBaldo September 16, 2024 19:45

broadbot temporarily deployed to pr-8 September 16, 2024 19:59 Inactive

lucymcnatt reviewed Sep 16, 2024

View reviewed changes

mlilynolting reviewed Sep 17, 2024

View reviewed changes

lint

7fd7634

jdcanas force-pushed the jc-compute-modal-types branch from 6fac7be to 7fd7634 Compare September 17, 2024 15:37

lint

84b539e

broadbot temporarily deployed to pr-8 September 17, 2024 16:02 Inactive

tests

192644e

jdcanas requested review from lucymcnatt and mlilynolting September 17, 2024 19:50

fix tests

9fd4766

jdcanas commented Sep 18, 2024

View reviewed changes

broadbot temporarily deployed to pr-8 September 18, 2024 16:22 Inactive

pr feedback

676b01a

broadbot temporarily deployed to pr-8 September 19, 2024 15:57 Inactive

mlilynolting approved these changes Sep 19, 2024

View reviewed changes

cleanup

96cf679

broadbot temporarily deployed to pr-8 September 19, 2024 17:31 Inactive

LizBaldo approved these changes Sep 19, 2024

View reviewed changes

mlilynolting approved these changes Sep 20, 2024

View reviewed changes

lucymcnatt approved these changes Sep 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IA-4137] Add types to desired and existing runtime config #5079

[IA-4137] Add types to desired and existing runtime config #5079

jdcanas commented Sep 12, 2024

msilva-broad Sep 12, 2024

jdcanas Sep 13, 2024

msilva-broad Sep 12, 2024

jdcanas Sep 13, 2024

LizBaldo Sep 13, 2024

jdcanas Sep 13, 2024

LizBaldo Sep 13, 2024

jdcanas Sep 13, 2024

LizBaldo Sep 13, 2024

lucymcnatt left a comment

lucymcnatt Sep 16, 2024

jdcanas Sep 17, 2024

mlilynolting Sep 18, 2024

mlilynolting Sep 18, 2024

jdcanas Sep 19, 2024

jdcanas Sep 19, 2024

lucymcnatt Sep 16, 2024

jdcanas Sep 17, 2024

mlilynolting left a comment

jdcanas commented Sep 17, 2024

jdcanas commented Sep 17, 2024

mlilynolting commented Sep 18, 2024

jdcanas Sep 18, 2024 •

edited

Loading

sonarcloud bot commented Sep 19, 2024

		autopauseThreshold: currentRuntimeDetails?.autopauseThreshold,
		computeConfig,

[IA-4137] Add types to desired and existing runtime config #5079

Are you sure you want to change the base?

[IA-4137] Add types to desired and existing runtime config #5079

Conversation

jdcanas commented Sep 12, 2024

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

lucymcnatt left a 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

mlilynolting left a comment

Choose a reason for hiding this comment

jdcanas commented Sep 17, 2024

jdcanas commented Sep 17, 2024

mlilynolting commented Sep 18, 2024

jdcanas Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Sep 19, 2024

Quality Gate passed

jdcanas Sep 18, 2024 •

edited

Loading