Handle invalid locked k8s version field #5205

ericpromislow · 2023-07-20T22:21:49Z

More checks that any specified k8s field is valid, and doesn't conflict with the locked field, if given.

And also verify that if the k8s version field is locked, that it's also valid.

Waiting on PR 5199

pkg/rancher-desktop/backend/backendHelper.ts

rak-phillip

LGTM. Tested in Linux and was alerted that the version in my locked file was invalid and that the most recent stable version of 1.27.3 would be used instead.

bats/tests/helpers/vm.bash

bats/tests/k8s/specify-invalid-k8s-version.bats

bats/tests/helpers/vm.bash

jandubois

The logic looks fine to me, but is it possible to check the behaviour with BATS or e2e tests, so we don't break it in the future when we refactor stuff?

bats/tests/helpers/vm.bash

jandubois

This PR really needs to include a BATS test that verifies all the situations it is supposed to cover.

ericpromislow · 2023-07-27T00:41:37Z

Actually writing the BATs test uncovered a case where an invalid locked k8s version was quietly changed to a valid one, so good show.

And then I realized we need a BATs test where a specified valid k8s version on the command-line != the locked k8s version.

jandubois

Thanks for adding the test!

I did a quick review of the new BATS test, just based on how the helper functions work. I haven't verified yet that the test covers all the acceptance criteria; will do that tomorrow.

bats/tests/profile/invalid-locked-k8s-version.bats

background.ts

pkg/rancher-desktop/backend/lima.ts

pkg/rancher-desktop/backend/backendHelper.ts

Signed-off-by: Eric Promislow <[email protected]>

…ock profile. Also add a BATs to verify this is implemented. Signed-off-by: Eric Promislow <[email protected]>

…valid locked one. Signed-off-by: Eric Promislow <[email protected]>

- Don't write tests that rely on marshalled exception objects in the log files. Rely on handcrafted messages. Signed-off-by: Eric Promislow <[email protected]>

jandubois

This looks fine to me now, but I would like @rak-phillip to review the TypeScript code one more time before we merge this!

rak-phillip

I think these changes look good, I just have one minor comment to address and then I think we're good to merge.

rak-phillip · 2023-08-02T00:02:55Z

pkg/rancher-desktop/backend/backendHelper.ts

    let storedVersion: semver.SemVer|null;
    let matchedVersion: semver.SemVer|undefined;
    const invalidK8sVersionMainMessage = `Requested kubernetes version '${ currentConfigVersionString }' is not a valid version.`;
+    const sv = new SettingsValidator();
+    const lockedSettings = settingsImpl.getLockedSettings();
+    const versionIsLocked = lockedSettings.kubernetes?.version ?? false;


Would we want to convert lockedSettings.kubernetes.version to its boolean representation via something like const versionIsLocked = !!lockedSettings.kubernetes?.version ?? false;? I know that truthy and falsy values are entirely valid, but doing the conversion more explicitly communicates what we are planning to do with this value later on.

That isn't necessary. While the type of lockedSettings is Record<string, any>, it's like that because I still never get recursive typescript types right -- it should be

type LockedSettingsType = Record<string, boolean | LockedSettingsType>

So all references are either boolean or null (the expression isn't represented in the current object), and the expression as above is always boolean without needing a !!.

gaktive requested a review from rak-phillip July 21, 2023 18:09

ericpromislow marked this pull request as draft July 21, 2023 22:58

ericpromislow force-pushed the 4952-handle-invalid-k8s-version branch from b323b91 to 37eb237 Compare July 21, 2023 23:11

ericpromislow force-pushed the 5203-handle-invalid-locked-k8s-versions branch from ce28d28 to 80ac720 Compare July 21, 2023 23:30

github-advanced-security bot found potential problems Jul 21, 2023

View reviewed changes

pkg/rancher-desktop/backend/backendHelper.ts Fixed Show fixed Hide fixed

Base automatically changed from 4952-handle-invalid-k8s-version to main July 24, 2023 16:51

ericpromislow force-pushed the 5203-handle-invalid-locked-k8s-versions branch from 8c03609 to e7c0e06 Compare July 24, 2023 16:52

ericpromislow marked this pull request as ready for review July 24, 2023 16:53

jandubois mentioned this pull request Jul 24, 2023

Deployment profiles are ignoring Kubernetes version wrong data type #4952

Closed

rak-phillip previously approved these changes Jul 24, 2023

View reviewed changes

ericpromislow force-pushed the 5203-handle-invalid-locked-k8s-versions branch from e7c0e06 to 073d22c Compare July 24, 2023 23:29

jandubois reviewed Jul 25, 2023

View reviewed changes

bats/tests/helpers/vm.bash Outdated Show resolved Hide resolved

jandubois reviewed Jul 25, 2023

View reviewed changes

bats/tests/k8s/specify-invalid-k8s-version.bats Outdated Show resolved Hide resolved

gaktive requested a review from jandubois July 25, 2023 17:43

ericpromislow dismissed rak-phillip’s stale review via 49899fe July 25, 2023 18:22

ericpromislow force-pushed the 5203-handle-invalid-locked-k8s-versions branch 3 times, most recently from 3a49e75 to 480ed95 Compare July 25, 2023 22:02

jandubois reviewed Jul 26, 2023

View reviewed changes

bats/tests/helpers/vm.bash Outdated Show resolved Hide resolved

jandubois requested changes Jul 26, 2023

View reviewed changes

bats/tests/helpers/vm.bash Outdated Show resolved Hide resolved

ericpromislow force-pushed the 5203-handle-invalid-locked-k8s-versions branch from 480ed95 to 3a3d8b8 Compare July 26, 2023 18:28

ericpromislow requested review from jandubois and rak-phillip July 26, 2023 18:28

jandubois requested changes Jul 26, 2023

View reviewed changes

ericpromislow force-pushed the 5203-handle-invalid-locked-k8s-versions branch from 3a3d8b8 to 5f9e67d Compare July 27, 2023 00:16

ericpromislow requested a review from jandubois July 27, 2023 00:41

jandubois requested changes Jul 27, 2023

View reviewed changes

ericpromislow requested a review from jandubois July 27, 2023 17:46

ericpromislow force-pushed the 5203-handle-invalid-locked-k8s-versions branch from ca37357 to f56df81 Compare July 27, 2023 18:25

ericpromislow force-pushed the 5203-handle-invalid-locked-k8s-versions branch from 618958f to 4695b4d Compare July 28, 2023 20:15

jandubois reviewed Jul 31, 2023

View reviewed changes

bats/tests/profile/invalid-locked-k8s-version.bats Show resolved Hide resolved

ericpromislow force-pushed the 5203-handle-invalid-locked-k8s-versions branch from 4695b4d to 818cbda Compare August 1, 2023 18:22

github-advanced-security bot found potential problems Aug 1, 2023

View reviewed changes

background.ts Fixed Show fixed Hide fixed

background.ts Fixed Show fixed Hide fixed

background.ts Fixed Show fixed Hide fixed

background.ts Fixed Show fixed Hide fixed

pkg/rancher-desktop/backend/lima.ts Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Aug 1, 2023

View reviewed changes

pkg/rancher-desktop/backend/backendHelper.ts Fixed Show fixed Hide fixed

ericpromislow added 4 commits August 1, 2023 13:11

Fatal error if a locked k8s.version field is invalid.

0e4cf79

Signed-off-by: Eric Promislow <[email protected]>

Different error message if an invalid k8s version is specified in a l…

f732a4d

…ock profile. Also add a BATs to verify this is implemented. Signed-off-by: Eric Promislow <[email protected]>

Add a BATs test where a specified valid k8s version conflicts with a …

0227d96

…valid locked one. Signed-off-by: Eric Promislow <[email protected]>

Make testing with exceptions more resilient.

726884b

- Don't write tests that rely on marshalled exception objects in the log files. Rely on handcrafted messages. Signed-off-by: Eric Promislow <[email protected]>

ericpromislow force-pushed the 5203-handle-invalid-locked-k8s-versions branch from 966ba1e to 726884b Compare August 1, 2023 20:15

ericpromislow requested a review from jandubois August 1, 2023 23:05

jandubois approved these changes Aug 1, 2023

View reviewed changes

rak-phillip reviewed Aug 2, 2023

View reviewed changes

rak-phillip merged commit 2816514 into main Aug 2, 2023

rak-phillip deleted the 5203-handle-invalid-locked-k8s-versions branch August 2, 2023 17:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle invalid locked k8s version field #5205

Handle invalid locked k8s version field #5205

ericpromislow commented Jul 20, 2023 •

edited

Loading

rak-phillip left a comment

jandubois left a comment

jandubois left a comment

ericpromislow commented Jul 27, 2023

jandubois left a comment

jandubois left a comment

rak-phillip left a comment

rak-phillip Aug 2, 2023

ericpromislow Aug 2, 2023

Handle invalid locked k8s version field #5205

Handle invalid locked k8s version field #5205

Conversation

ericpromislow commented Jul 20, 2023 • edited Loading

Waiting on PR 5199

rak-phillip left a comment

Choose a reason for hiding this comment

jandubois left a comment

Choose a reason for hiding this comment

jandubois left a comment

Choose a reason for hiding this comment

ericpromislow commented Jul 27, 2023

jandubois left a comment

Choose a reason for hiding this comment

jandubois left a comment

Choose a reason for hiding this comment

rak-phillip left a comment

Choose a reason for hiding this comment

rak-phillip Aug 2, 2023

Choose a reason for hiding this comment

ericpromislow Aug 2, 2023

Choose a reason for hiding this comment

ericpromislow commented Jul 20, 2023 •

edited

Loading