Finish reading deployment profiles from the Windows registry #4251

ericpromislow · 2023-03-21T23:16:22Z

Fixes #4153
Fixes #4193

mook-as

This PR is missing tests. Actually, we should have tests in a separate PR, and merge that before this one.

pkg/rancher-desktop/main/deploymentProfiles.ts

mook-as

Unflagging for review because this keeps being changed

ericpromislow · 2023-05-05T19:36:49Z

Keep in mind that the follow-up PR #4612 will display deployment profile type-errors in a dialog box and then shut down.

This PR is about doing a better job of reading deployment profiles, and it also recognizes the "special objects", like WSL.integrations

pkg/rancher-desktop/main/deploymentProfiles.ts

mook-as · 2023-05-08T23:09:00Z

pkg/rancher-desktop/main/deploymentProfiles.ts

+  registryKeysFixedCase.forEach((key, index) => {
+    const originalKey = registryKeys[index];


We don't seem to ever need index again, and registryKeysFixedCase is only used here.
I think the code would be a lot easier to understand as:

Suggested change

registryKeysFixedCase.forEach((key, index) => {

const originalKey = registryKeys[index];

for (const originalKey of Object.keys(schemaObj)) {

const key = fixProfileKeyCase(originalKey, schemaKeys);

Looking at the loop over commonKeys, it's probably better to call these schemaKey and registryKey, too.

With this approach, you're not going to find keys that are in the registry instance but aren't in the schema.

In that case, how about:

for (const originalKey of nativeReg.enumKeyNames(regKey)) { const key = fixProfileKeyCase(originalKey, schemaKeys);

(You'll notice that my original code is wrong anyway, passing a key of schemaObj to fixProfileKeyCase doesn't make sense…)

pkg/rancher-desktop/main/deploymentProfiles.ts

pkg/rancher-desktop/main/__tests__/deploymentProfiles.spec.ts

mook-as · 2023-05-09T00:25:59Z

pkg/rancher-desktop/main/deploymentProfiles.ts

+  const registryValueNames = nativeReg.enumValueNames(regKey);
+  const registryValueNamesFixedCase = registryValueNames.map(k => fixProfileKeyCase(k, schemaKeys));
+
+  registryValueNamesFixedCase.forEach((key, index) => {


Same as above — for (const originalName of nativeReg.enumValueNames(regKey) { const key = fixProfileKeyCase(originalName, schemaKeys);

And same as above, if we walk the schema instead of the registry we miss undefined entries

Ah! My suggestion here was correct (enumerating over the result of enumValueNames).

pkg/rancher-desktop/main/deploymentProfiles.ts

mook-as

A lot of this type checking makes me think we should just pull in the schema validator we already use for the HTTP API. We may need to massage it to be usable here, though. Also, that can be a follow-up PR.

pkg/rancher-desktop/main/__tests__/deploymentProfiles.spec.ts

pkg/rancher-desktop/main/deploymentProfiles.ts

pkg/rancher-desktop/main/__tests__/deploymentProfiles.spec.ts

mook-as

Nothing major left, just some nits. The main changes are just in the test.

mook-as · 2023-05-12T16:27:11Z

pkg/rancher-desktop/main/deploymentProfiles.ts

+    // Drop the initial 'defaults' or 'locked' field
+    const pathPartsWithoutHiveType = pathParts.slice(1);
+
+    for (const originalKey of nativeReg.enumKeyNames(regKey)) {


Suggested change

for (const originalKey of nativeReg.enumKeyNames(regKey)) {

for (const registryKey of nativeReg.enumKeyNames(regKey)) {

Since that makes it easier when we push to userDefinedObjectKeys/commonKeys.

pkg/rancher-desktop/main/deploymentProfiles.ts

pkg/rancher-desktop/main/__tests__/deploymentProfiles.spec.ts

- Don't complain about any entries in the settings schema that aren't in the registry. - Treat `WSL/integrations`, `extensions`, and `diagnostics/mutedChecks` as hashes - Registry keys and value names are case-insensitive - Do type-based parsing of raw registry values. - There's no need to do post-input validation of registry data. The reader validates as it goes. - Convert 0s and 1s in user-defined objects into booleans. Currently if there are certain errors in a deployment profile, such as unrecognized names or type mismatches, we log them but they aren't fatal, and aren't shown in a dialog box. These will be raised into dialog boxes with fatal errors in a subsequent PR. Signed-off-by: Eric Promislow <[email protected]>

- Use case-sensitive checks for user-defined objects. We've already normalized the case of the registry field, so can use === to compare. - The reg file can be ascii-encoded. Any non-ascii characters should appear only inside SZ strings and be hex-encoded. All the names in the schema can be represented in 7-bit ASCII characters so there's no need to support other encodings. Signed-off-by: Eric Promislow <[email protected]>

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

mook-as

While I have a couple comments, they don't block merging.

mook-as · 2023-05-12T20:10:06Z

pkg/rancher-desktop/main/__tests__/deploymentProfiles.spec.ts

+    await fs.promises.rm(testDir, { force: true, recursive: true });
+    consoleMock.mockReset();
+  });
+  // TODO:  Add an `afterAll(clearRegistry)` when we're finished developing.


Does that mean we should do it before the PR merges?

No, there are still at least two more PRs to come after this one.

mook-as · 2023-05-12T20:14:32Z

pkg/rancher-desktop/main/deploymentProfiles.ts

-
-  return false;
+function haveUserDefinedObject(pathParts: string[]): boolean {
+  return userDefinedKeys.some(userDefinedKey => _.isEqual(userDefinedKey, pathParts));


The non-lodash way would be

const userDefinedKeys = ['extensions', 'WSL.integrations', 'diagnostics.mutedChecks']; function haveUserDefinedObject(pathParts: string[]): boolean { return userDefinedKeys.includes(pathParts.join('.')); }

But then you're always joining an array to create a string, and I'm going to assume lodash's way of doing the comparison will be faster, since it will eliminate different-size arrays and fail on the different in the first string right away.

The diff is probably miniscule in the scheme of things anyway.

ericpromislow requested a review from mook-as March 21, 2023 23:16

ericpromislow marked this pull request as draft March 22, 2023 00:06

ericpromislow marked this pull request as ready for review March 22, 2023 00:16

ericpromislow mentioned this pull request Mar 22, 2023

Windows Deployment profile registry key types get auto corrected #4218

Closed

mook-as requested changes Mar 22, 2023

View reviewed changes

ericpromislow force-pushed the 4193-registry-profile-improvements branch from a97fdca to 1691e41 Compare March 23, 2023 23:32

ericpromislow marked this pull request as draft March 24, 2023 18:33

ericpromislow force-pushed the 4193-registry-profile-improvements branch from 7bca028 to d2f8831 Compare April 27, 2023 21:45

ericpromislow changed the base branch from main to 4209-handle-bad-linux-deployment-files April 27, 2023 21:59

ericpromislow force-pushed the 4209-handle-bad-linux-deployment-files branch from 16cb99e to f634529 Compare April 28, 2023 18:50

ericpromislow force-pushed the 4193-registry-profile-improvements branch from 53c0ac5 to ea0a4f1 Compare April 28, 2023 20:33

ericpromislow changed the base branch from 4209-handle-bad-linux-deployment-files to 4579-load-user-defined-deployment-settings April 28, 2023 20:33

ericpromislow force-pushed the 4193-registry-profile-improvements branch 2 times, most recently from 0133e77 to 27bb153 Compare April 28, 2023 23:15

ericpromislow force-pushed the 4579-load-user-defined-deployment-settings branch from 23a2b17 to ab9f223 Compare April 28, 2023 23:34

ericpromislow force-pushed the 4193-registry-profile-improvements branch from 27bb153 to 2ee51de Compare April 28, 2023 23:34

ericpromislow requested a review from mook-as April 28, 2023 23:38

ericpromislow force-pushed the 4193-registry-profile-improvements branch from 67dd43a to 72e6f2e Compare April 29, 2023 00:12

ericpromislow marked this pull request as ready for review May 1, 2023 17:47

ericpromislow marked this pull request as draft May 1, 2023 18:00

ericpromislow force-pushed the 4579-load-user-defined-deployment-settings branch from ab9f223 to a6c769d Compare May 1, 2023 21:30

ericpromislow force-pushed the 4193-registry-profile-improvements branch from bcdfafc to 0f42874 Compare May 2, 2023 19:28

Base automatically changed from 4579-load-user-defined-deployment-settings to main May 2, 2023 20:21

ericpromislow force-pushed the 4193-registry-profile-improvements branch 3 times, most recently from 1e97704 to 03bc6a0 Compare May 3, 2023 00:08

ericpromislow marked this pull request as ready for review May 3, 2023 00:09

ericpromislow force-pushed the 4193-registry-profile-improvements branch from 03bc6a0 to 9c0c48a Compare May 3, 2023 00:22

ericpromislow marked this pull request as draft May 3, 2023 17:03

ericpromislow marked this pull request as ready for review May 3, 2023 18:46

ericpromislow marked this pull request as draft May 3, 2023 18:51

ericpromislow force-pushed the 4193-registry-profile-improvements branch 2 times, most recently from 3a79099 to 2e11979 Compare May 3, 2023 19:49

ericpromislow marked this pull request as ready for review May 3, 2023 19:52

ericpromislow force-pushed the 4193-registry-profile-improvements branch from 2e11979 to fe879d4 Compare May 4, 2023 17:06

mook-as reviewed May 4, 2023

View reviewed changes

ericpromislow force-pushed the 4193-registry-profile-improvements branch 3 times, most recently from 0c6faec to a9bba89 Compare May 5, 2023 19:18

ericpromislow requested a review from mook-as May 5, 2023 19:35

mook-as requested changes May 9, 2023

View reviewed changes

ericpromislow force-pushed the 4193-registry-profile-improvements branch 2 times, most recently from 4b25702 to 2f50bbc Compare May 9, 2023 19:43

ericpromislow requested a review from mook-as May 9, 2023 21:52

mook-as requested changes May 11, 2023

View reviewed changes

ericpromislow force-pushed the 4193-registry-profile-improvements branch from c7f0b22 to d8b9be9 Compare May 12, 2023 01:01

ericpromislow requested a review from mook-as May 12, 2023 01:01

mook-as requested changes May 12, 2023

View reviewed changes

ericpromislow force-pushed the 4193-registry-profile-improvements branch from 8d35f55 to cbfd7bf Compare May 12, 2023 18:18

ericpromislow added 2 commits May 12, 2023 11:19

ericpromislow force-pushed the 4193-registry-profile-improvements branch from cbfd7bf to cf06fdc Compare May 12, 2023 18:20

Remove the reliance on particular order of error messages.

7dc893d

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

ericpromislow requested a review from mook-as May 12, 2023 18:49

mook-as approved these changes May 12, 2023

View reviewed changes

ericpromislow merged commit 7a2d611 into main May 12, 2023

jandubois deleted the 4193-registry-profile-improvements branch June 12, 2023 23:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finish reading deployment profiles from the Windows registry #4251

Finish reading deployment profiles from the Windows registry #4251

ericpromislow commented Mar 21, 2023 •

edited

Loading

mook-as left a comment

mook-as left a comment

ericpromislow commented May 5, 2023

mook-as May 8, 2023

ericpromislow May 9, 2023

mook-as May 11, 2023

mook-as May 9, 2023

ericpromislow May 9, 2023

mook-as May 11, 2023

mook-as left a comment

mook-as left a comment

mook-as May 12, 2023

ericpromislow May 12, 2023

mook-as left a comment

mook-as May 12, 2023

ericpromislow May 12, 2023

mook-as May 12, 2023

ericpromislow May 12, 2023

		registryKeysFixedCase.forEach((key, index) => {
		const originalKey = registryKeys[index];

	for (const originalKey of nativeReg.enumKeyNames(regKey)) {
	for (const registryKey of nativeReg.enumKeyNames(regKey)) {

Finish reading deployment profiles from the Windows registry #4251

Finish reading deployment profiles from the Windows registry #4251

Conversation

ericpromislow commented Mar 21, 2023 • edited Loading

mook-as left a comment

Choose a reason for hiding this comment

mook-as left a comment

Choose a reason for hiding this comment

ericpromislow commented May 5, 2023

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

mook-as left a comment

Choose a reason for hiding this comment

mook-as left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mook-as 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

ericpromislow commented Mar 21, 2023 •

edited

Loading