Add a --output=reg|json option to `rdctl list-settings` #5006

ericpromislow · 2023-06-21T00:37:01Z

Options are --output=reg[,hklm|hkcu][,defaults|locked]

defaults are hklm and defaults

Output is written to stdout

To run unit tests:

cd src/go/rdctl/pkg/reg
go test .

jandubois · 2023-06-21T20:29:17Z

The E2E test is failing:

-   "stdout": StringContaining "[HKEY_LOCAL_MACHINE\\SOFTWARE\\Rancher Desktop\\Profile\\defaults]",
...
+ [HKEY_LOCAL_MACHINE\\SOFTWARE\\Profiles\\Rancher Desktop\\defaults]

The Profiles substring is in a different place.

Note that the expected string specifies the "legacy" location from the 1.8 release.

The actual string in the output does not use the correct current location, which uses Policies in place of Profiles:

[HKEY_LOCAL_MACHINE\\SOFTWARE\\Policies\\Rancher Desktop\\defaults]

jandubois

Please generate the correct current profile locations in the *.reg files, and fix the E2E test to match.

ericpromislow · 2023-06-21T20:33:13Z

I moved the e2e tests to bats tests in preferences/list-settings-output.bats , but the assert-output
test gives a false pass at

rancher-desktop/bats/tests/preferences/list-settings-output.bats

Line 60 in 94ca018

assert_output <<'EOF'

If I add some nonsense to any line after the first line of the heredoc, the test still passes. The docs in bats/bats-assert/README.md talk about displaying failures for multi-line strings in multi-line format, but I'm not seeing that happen.

ericpromislow · 2023-06-21T21:17:47Z

Please generate the correct current profile locations in the *.reg files, and fix the E2E test to match.

Done. This is why we have reviews.

Fixed in commit b9997f0

mook-as

Do we need to output version in the registry? I can see it going either way…

mook-as · 2023-06-23T18:02:03Z

src/go/rdctl/cmd/listSettings.go

 func init() {
 	rootCmd.AddCommand(listSettingsCmd)
+	listSettingsCmd.Flags().StringVarP(&outputSettings, "output", "", "", "output format: json|reg[,hive][,type], default json")


This is a bit confusing to parse. Would it be possible to make hive and type their own parameters (that are ignored unless --output=reg) instead? I'm imagining something like rdctl list-settings --output=reg --hive=hkcu --locked.

This sounds better, except it should be an error if --hive and/or --locked or--defaults are specified and --output=reg isn't. @jandubois ?

As for the version field, I still think doing a conversion would be more useful than having to manually edit (or process) a generated reg file to pull out only the lines the user cares about.

src/go/rdctl/cmd/listSettings.go

src/go/rdctl/pkg/reg/reg.go

mook-as · 2023-06-23T18:50:49Z

src/go/rdctl/pkg/reg/reg.go

+//	an array of lines representing the current value
+//	boolean: true if the current entry is empty
+//	error
+func convertToRegFormat(pathParts []string, v reflect.Value, jsonTag string) ([]string, bool, error) {


The doc block for this function is… very atypical for golang. Would it be possible to format it better so that it's more readable?

I looked up godoc, and don't see a conventional way to document either the parameters or the return values, so I expanded it a bit.

mook-as · 2023-06-23T18:53:22Z

src/go/rdctl/pkg/reg/reg.go

+/**
+ * @param hiveType: "hklm" or "hkcu"
+ * @param profileType: "defaults" or "locked"
+ * @param settingsBodyAsJSON - options marshaled as JSON


This would be clearer if we made the caller do the unmarshalling, I think. If we make this generic (that's an option now, or we use interface{}), we might even be able to test with structs that are limited by the settings format.

Maybe I misunderstood this comment, but after spending a bit of time on it I disagree.

The problem with this approach is that it makes writing tests onerous without a code-generation tool.

It's easy to both write and read a block of JSON code for a specific test, and verify the output. But if I have to create a structure, I'll have to assign each constant value to a variable, and then assign pointers to each variable that we care about. It's time-consuming to do that, and tedious for anyone to read the code.

The result is the same whether we do the unmarshaling in the caller or in the JsonToReg function, but overall
it's simpler and clearer to do the unmarshaling in the latter.

Talking about generalizing the types of structures we can pass to JsonToReg is YAGNI right now. Right now I don't reflect arrays of strings correctly, getting a ucs2 encoding of "<interface {} Value> <interface {} Value> <interface {} Value>" instead of the expected "abc" "def" "ghi", but that's because when I see an array in the reflector, I'm assuming it's an array of strings.

Also the generic converter now intermixes scalars and nested structs, which breaks the registry format, because all the scalar entries have to appear immediately after their containing struct's header. No point trying to figure out why this is happening unless and until we decide to generalize the converter.

Note that the unmarshaled struct will still need to have JSON tags in order to generate registry values.

mook-as · 2023-06-23T20:33:17Z

src/go/rdctl/pkg/reg/reg.go

+	fullHiveType, ok := map[string]string{"hklm": "HKEY_LOCAL_MACHINE", "hkcu": "HKEY_CURRENT_USER"}[hiveType]
+	if !ok {
+		return nil, fmt.Errorf("unrecognized hiveType of '%s', must be 'hklm' or 'hkcu'", hiveType)
+	}


It would be good to allow case-insensitivity here (and for the other flag). But again, we should parse this elsewhere and just get passed in clean options.

I implemented what was spec'ed in the issue. Jan opened it up for comments, and I was ok with it -- we should discuss this there, possibly in a separate issue, rather than holding up this work.

I made the right-side parts of --section= and --reg-hive case-insensitive, but all the help etc. will be in terms of the lower-case forms.

src/go/rdctl/pkg/reg/reg.go

src/go/rdctl/cmd/listSettings.go

github-advanced-security

check-spelling found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

src/go/rdctl/pkg/reg/reg.go

src/go/rdctl/pkg/reg/reg_test.go

bats/tests/preferences/list-settings-output.bats

mook-as

Thanks for the changes! Just some relatively trivial things left.

src/go/rdctl/cmd/listSettings.go

mook-as · 2023-07-04T16:56:15Z

src/go/rdctl/cmd/listSettings.go

 func init() {
 	rootCmd.AddCommand(listSettingsCmd)
+	listSettingsCmd.Flags().StringVarP(&outputSettingsFlags.Format, "output", "", jsonFormat, fmt.Sprintf("output format: %s|%s, default %s", jsonFormat, regFormat, jsonFormat))


I don't think you need to list the default here:

--output string output format: json|reg, default json (default "json")

mook-as · 2023-07-04T16:58:20Z

src/go/rdctl/cmd/listSettings.go

 func init() {
 	rootCmd.AddCommand(listSettingsCmd)
+	listSettingsCmd.Flags().StringVarP(&outputSettingsFlags.Format, "output", "", jsonFormat, fmt.Sprintf("output format: %s|%s, default %s", jsonFormat, regFormat, jsonFormat))
+	listSettingsCmd.Flags().StringVarP(&outputSettingsFlags.RegistryHive, "reg-hive", "", "", fmt.Sprintf("registry hive: %s|%s, default %s", reg.HklmRegistryHive, reg.HkcuRegistryHive, reg.HklmRegistryHive))


Ah, we're not setting defaults here because they don't apply to JSON (same with profile type)? Can't we just set a default here, and ignore the value if we're using JSON?

I'd rather treat rdctl list-settings --output=json --reg-hive=hklm as an error because the output won't meet the user's expectations (whatever they might be).

src/go/rdctl/cmd/listSettings.go

This is used to convert current settings into Windows registry file format. * Generate a full options struct in golang representing all possible preference fields, including: * arrays * maps * fields used only for other platforms. This is needed for this branch in order to convert JSON documents containing the above constructs into reg-files, even if those features aren't supported on the current platform. This does not add support for specifying array or map values to `rdctl set` or `rdctl start`, but is a pre-rec for those features. * Reg files need to have scalar values appearing before nested structs. Otherwise, if scalar values appear after a nested struct is written out, it looks like the scalar belongs to the last nested struct, not the one it actually belongs to. Signed-off-by: Eric Promislow <epromislow@suse.com>

Signed-off-by: Eric Promislow <epromislow@suse.com>

Basically, len(returnedLines) == 0 === isEmpty Signed-off-by: Eric Promislow <epromislow@suse.com>

Signed-off-by: Eric Promislow <epromislow@suse.com>

…=reg. Signed-off-by: Eric Promislow <epromislow@suse.com>

Signed-off-by: Eric Promislow <epromislow@suse.com>

- Add windows-registry-specific type terminology to `allow.txt` - Stop using nonsense words in tests. Signed-off-by: Eric Promislow <epromislow@suse.com>

…section`. Noted that the allowed values for `--output` are case-*sensitive*. The difference is that it's helpful if the user can use the same case that the registry uses, namely all-caps for `--reg-hive` (e.g. HKLM_LOCAL_MACHINE`), and capitalized for `--section` (like 'Defaults'). `reg` and `json` are names of output-formats that we are proposing. Also I don't convert the names to all-lower-case until I know we're using them, so I have the original case if I need to put the entered string in an error message. Signed-off-by: Eric Promislow <epromislow@suse.com>

Signed-off-by: Eric Promislow <epromislow@suse.com>

…tivity. Signed-off-by: Eric Promislow <epromislow@suse.com>

Signed-off-by: Eric Promislow <epromislow@suse.com>

ericpromislow requested a review from mook-as June 21, 2023 00:37

jandubois previously requested changes Jun 21, 2023

View reviewed changes

ericpromislow marked this pull request as draft June 21, 2023 20:48

ericpromislow marked this pull request as ready for review June 21, 2023 21:10

ericpromislow requested a review from jandubois June 21, 2023 21:17

ericpromislow force-pushed the 4562-add-reg-output-option branch from d2f4962 to cb6bd7b Compare June 21, 2023 23:17

ericpromislow mentioned this pull request Jun 21, 2023

Support generating reg files from arbitrary JSON snippets #5020

Closed

ericpromislow force-pushed the 4562-add-reg-output-option branch from cb6bd7b to 17e6b3b Compare June 22, 2023 19:29

jandubois force-pushed the 4562-add-reg-output-option branch from 17e6b3b to 0bbcc5d Compare June 23, 2023 19:10

mook-as requested changes Jun 23, 2023

View reviewed changes

mook-as mentioned this pull request Jun 23, 2023

Add REG output option to rdctl settings #4562

Closed

ericpromislow force-pushed the 4562-add-reg-output-option branch 2 times, most recently from 11afc64 to 4dec4e4 Compare June 24, 2023 00:03

ericpromislow requested a review from mook-as June 24, 2023 00:17

mook-as requested changes Jun 28, 2023

View reviewed changes

src/go/rdctl/cmd/listSettings.go Outdated Show resolved Hide resolved

ericpromislow force-pushed the 4562-add-reg-output-option branch from 59ce8b1 to 8b652c4 Compare June 29, 2023 22:26

github-advanced-security bot found potential problems Jun 29, 2023

View reviewed changes

ericpromislow requested a review from mook-as June 29, 2023 22:38

ericpromislow force-pushed the 4562-add-reg-output-option branch from e353a8c to 69c18a0 Compare July 3, 2023 21:18

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

View reviewed changes

bats/tests/preferences/list-settings-output.bats Fixed Show fixed Hide fixed

mook-as requested changes Jul 4, 2023

View reviewed changes

ericpromislow added 4 commits July 4, 2023 14:28

Add tests to verify --output reg options are handled correctly.

c9fd444

Signed-off-by: Eric Promislow <epromislow@suse.com>

Constantize some of the strings.

9563a5b

Signed-off-by: Eric Promislow <epromislow@suse.com>

Improve the documentation.

772a513

Signed-off-by: Eric Promislow <epromislow@suse.com>

ericpromislow added 13 commits July 4, 2023 14:28

The linter was complaining about this empty slice.

271954e

Signed-off-by: Eric Promislow <epromislow@suse.com>

Stop putting empty lines before registry-file header lines.

206d6b6

Signed-off-by: Eric Promislow <epromislow@suse.com>

The linter wanted these changes.

ca99a11

Signed-off-by: Eric Promislow <epromislow@suse.com>

No need for both isEmpty and returnedLines.

dd5a2c5

Basically, len(returnedLines) == 0 === isEmpty Signed-off-by: Eric Promislow <epromislow@suse.com>

Undo 4dec4e4

ad23488

Signed-off-by: Eric Promislow <epromislow@suse.com>

Implement --reg-hive and --section options for list-settings --output…

3457f2c

…=reg. Signed-off-by: Eric Promislow <epromislow@suse.com>

Deal with the spellchecker.

7ad742a

Signed-off-by: Eric Promislow <epromislow@suse.com>

More spellcheck-driven changes.

5a6cf73

- Add windows-registry-specific type terminology to `allow.txt` - Stop using nonsense words in tests. Signed-off-by: Eric Promislow <epromislow@suse.com>

Fix the hex-string test results to match the changed input.

ff6d254

Signed-off-by: Eric Promislow <epromislow@suse.com>

Work with the spellchecker - avoid random casing to test case-insensi…

65ac673

…tivity. Signed-off-by: Eric Promislow <epromislow@suse.com>

Update help strings to use newer options.

03eaea9

Signed-off-by: Eric Promislow <epromislow@suse.com>

Have getListSettings return a string instead of []byte.

07445dd

Signed-off-by: Eric Promislow <epromislow@suse.com>

ericpromislow force-pushed the 4562-add-reg-output-option branch from 4276f4d to 07445dd Compare July 4, 2023 21:28

ericpromislow requested a review from mook-as July 4, 2023 21:29

mook-as approved these changes Jul 4, 2023

View reviewed changes

ericpromislow merged commit dd3e089 into main Jul 5, 2023
14 checks passed

ericpromislow deleted the 4562-add-reg-output-option branch July 5, 2023 17:12

sunilarjun mentioned this pull request Aug 23, 2023

Updating Command Reference Output - rdctl list-settings --help rancher-sandbox/docs.rancherdesktop.io#270

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a --output=reg|json option to `rdctl list-settings` #5006

Add a --output=reg|json option to `rdctl list-settings` #5006

ericpromislow commented Jun 21, 2023

jandubois commented Jun 21, 2023

jandubois left a comment

ericpromislow commented Jun 21, 2023

ericpromislow commented Jun 21, 2023

mook-as left a comment

mook-as Jun 23, 2023

ericpromislow Jun 23, 2023

mook-as Jun 23, 2023

ericpromislow Jun 23, 2023

mook-as Jun 23, 2023

ericpromislow Jun 23, 2023

mook-as Jun 23, 2023

ericpromislow Jun 23, 2023

ericpromislow Jul 3, 2023

github-advanced-security bot left a comment

mook-as left a comment

mook-as Jul 4, 2023

mook-as Jul 4, 2023

ericpromislow Jul 4, 2023 •

edited

Loading

Add a --output=reg|json option to rdctl list-settings #5006

Add a --output=reg|json option to rdctl list-settings #5006

Conversation

ericpromislow commented Jun 21, 2023

jandubois commented Jun 21, 2023

jandubois left a comment

Choose a reason for hiding this comment

ericpromislow commented Jun 21, 2023

ericpromislow commented Jun 21, 2023

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

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

github-advanced-security bot 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

ericpromislow Jul 4, 2023 • edited Loading

Choose a reason for hiding this comment

Add a --output=reg|json option to `rdctl list-settings` #5006

Add a --output=reg|json option to `rdctl list-settings` #5006

ericpromislow Jul 4, 2023 •

edited

Loading