Implement `rdctl create-profile [ --input FILE | --body JSON | --from-settings ]` #5021

ericpromislow · 2023-06-21T23:26:00Z

This lets users create registry and plist files from either current settings or arbitrary JSON documents.

jandubois

Adding --body and --input options to list-settings doesn't make any sense; it should always fetch the current settings from the API.

If we want to have this general conversion functionality (and I do think that this is useful to have), then it needs to be a separate command.

Let's hash out the design of those commands first before working on the implementation.

ericpromislow · 2023-06-22T16:44:10Z

I agree that it doesn't make sense for list-settings to do this. But it didn't take much code - about 25 lines not counting doc-strings and tests, and I figured best to get it done while it was fresh in my mind.

I don't see how rdctl list-settings --output reg.... is useful unless you edit the output before importing it into the registry.

jandubois · 2023-06-22T19:31:30Z

I don't see how rdctl list-settings --output reg.... is useful unless you edit the output before importing it into the registry.

I agree, but removing stuff in an editor is still easier than writing a JSON input file manually. So I would still use the preferences dialog to set all the desired settings, export them into reg or plist format, and then just remove things I don't intend to lock down.

For the defaults profile it doesn't even matter; you can leave everything in. The defaults are only applied during first-run anyways.

bats/tests/preferences/generate-reg-output.bats

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

bats/tests/preferences/generate-reg-output.bats

bats/tests/profile/create-profile-output.bats

jandubois

This is only a partial review of the tests.

I would really like to understand the issue with assert_output and here-documents because I think a lot of the tests would read better with here-docs.

bats/tests/preferences/generate-reg-output.bats

bats/tests/profile/create-profile-output.bats

src/go/rdctl/cmd/createProfile.go

jandubois

Another round of feedback

bats/tests/profile/create-profile-output.bats

jandubois · 2023-07-28T01:54:21Z

bats/tests/profile/create-profile-output.bats

+    # Just match a few of the lines near the start and the end of the output.
+    # The unit tests do more comprehensive output checking.


The comment seems out-dated.

Yes, I'm going with full here-doc testing now.

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/plist/plist_test.go

jandubois

Another round of feedback, based solely on reading through the tests.

src/go/rdctl/cmd/createProfile.go

jandubois · 2023-07-28T23:00:49Z

src/go/rdctl/cmd/createProfile.go

+
+	if outputSettingsFlags.Format == plistFormat {
+		if outputSettingsFlags.RegistryHive != "" || outputSettingsFlags.RegistryProfileType != "" {
+			return fmt.Errorf("registry hive and type can't be specified with plist")


For consistency with quoting of all "values" in other errors:

Suggested change

return fmt.Errorf("registry hive and type can't be specified with plist")

return fmt.Errorf(`registry hive and type can't be specified with output "plist"`)

src/go/rdctl/cmd/createProfile.go

jandubois · 2023-07-28T23:03:09Z

src/go/rdctl/cmd/createProfile.go

+	case "":
+		outputSettingsFlags.RegistryProfileType = defaultsType
+	default:
+		return fmt.Errorf("invalid registry type of '%s' specified, must be %s or %s", outputSettingsFlags.RegistryProfileType, defaultsType, lockedType)


Suggested change

return fmt.Errorf("invalid registry type of '%s' specified, must be %s or %s", outputSettingsFlags.RegistryProfileType, defaultsType, lockedType)

return fmt.Errorf(`invalid registry type of "%s" specified, must be "%s" or "%s"`, outputSettingsFlags.RegistryProfileType, defaultsType, lockedType)

bats/tests/profile/create-profile-output.bats

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

Use double-quoting to quote all terms that need quoting in user-directed messages. Put command-line checking BATs tests before tests that run and check output. Signed-off-by: Eric Promislow <epromislow@suse.com>

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

On Windows run the output through 'reg import' to verify it's acceptable. Signed-off-by: Eric Promislow <epromislow@suse.com>

- The missing-file error message is different in WSL, so drop the specific part - The `<(function)` form doesn't work on Windows, so use it only on macos-specific tests. Signed-off-by: Eric Promislow <epromislow@suse.com>

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

Ensure that on the bash side we write to `/mnt/DRIVE/tmp`, and on Windows it's `DRIVE:\tmp`` We also have to change the directory name `Policies` to something else because modifying it requires admin privileges, even in HKCU. Signed-off-by: Eric Promislow <epromislow@suse.com>

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

- Put the bogus namespace in a variable - Use the term 'full settings' for consistency - Have `assert_check_registry_output` use `$output` Signed-off-by: Eric Promislow <epromislow@suse.com>

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

jandubois · 2023-08-02T00:44:50Z

bats/tests/profile/create-profile-output.bats

+@test "and shutdown" {
+    if is_macos; then
+        rdctl shutdown
+    fi
+}


This should not be necessary; we really should look into why this happens for you. Is this specific to using RD_LOCATION=dev? Because I always use RD_LOCATION=system, and it doesn't need a shutdown during teardown.

Will approve as is though and create a seoarate issue.

jandubois requested changes Jun 22, 2023

View reviewed changes

ericpromislow marked this pull request as draft June 22, 2023 16:44

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

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

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

ericpromislow force-pushed the 5020-fake-rdctl-convert-command branch from 3d9fd01 to 8a3cb6b Compare July 3, 2023 22:25

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

View reviewed changes

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

Base automatically changed from 4562-add-reg-output-option to main July 5, 2023 17:12

ericpromislow force-pushed the 5020-fake-rdctl-convert-command branch from b8b7925 to 628622b Compare July 7, 2023 00:35

ericpromislow force-pushed the 5020-fake-rdctl-convert-command branch 3 times, most recently from d346c31 to 6086479 Compare July 27, 2023 22:09

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

View reviewed changes

ericpromislow force-pushed the 5020-fake-rdctl-convert-command branch from 6086479 to cfae438 Compare July 27, 2023 22:30

ericpromislow changed the title ~~Add --input and --body flags to rdctl list-settings~~ Implement rdctl create-profile [ --input FILE | --body JSON | --from-settings ] Jul 27, 2023

ericpromislow marked this pull request as ready for review July 27, 2023 22:36

jandubois requested changes Jul 27, 2023

View reviewed changes

jandubois requested changes Jul 28, 2023

View reviewed changes

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

View reviewed changes

ericpromislow force-pushed the 5020-fake-rdctl-convert-command branch 2 times, most recently from 8328d0d to ab6770f Compare July 28, 2023 20:04

ericpromislow requested a review from jandubois July 28, 2023 20:04

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

View reviewed changes

src/go/rdctl/pkg/plist/plist_test.go Fixed Show fixed Hide fixed

src/go/rdctl/pkg/plist/plist_test.go Fixed Show fixed Hide fixed

jandubois requested changes Jul 29, 2023

View reviewed changes

ericpromislow added 17 commits August 1, 2023 16:14

Update spellcheck exceptions.

9082d04

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

Two consistence-based improvements:

6efb49a

Use double-quoting to quote all terms that need quoting in user-directed messages. Put command-line checking BATs tests before tests that run and check output. Signed-off-by: Eric Promislow <epromislow@suse.com>

Quoting and error-message fix.

30f6254

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

Fix bash variable names.

ef84731

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

Validate generated output on Windows.

ccc9f67

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

Output a here-doc instead of a multi-line echo argument.

ab46f3c

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

Drop full-profile-generation output checking.

ad456c1

On Windows run the output through 'reg import' to verify it's acceptable. Signed-off-by: Eric Promislow <epromislow@suse.com>

Changes to run on all platforms:

02aae6c

- The missing-file error message is different in WSL, so drop the specific part - The `<(function)` form doesn't work on Windows, so use it only on macos-specific tests. Signed-off-by: Eric Promislow <epromislow@suse.com>

Use a sample output that all platforms support.

22a5c59

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

Test special characters in Windows reg files.

205ac78

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

Appease the spellchecker.

95438c5

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

Apply shellcheck exception SC2001 against 'sed' locally.

afb3bd9

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

Have the header describe the test, and fill two gaps.

8a63ef9

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

More reviewer suggestions.

7034679

- Put the bogus namespace in a variable - Use the term 'full settings' for consistency - Have `assert_check_registry_output` use `$output` Signed-off-by: Eric Promislow <epromislow@suse.com>

Move the app-shutdown to the end.

53435ef

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

Add shellcheck directives due to use of '' at a distance from where set.

773e658

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

ericpromislow force-pushed the 5020-fake-rdctl-convert-command branch from 2d56f35 to 773e658 Compare August 1, 2023 23:46

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

ericpromislow added 6 commits August 1, 2023 17:23

More review-driven changes.

a6fcaa6

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

Remove a pipeline since this line isn't run on windows.

b7e6102

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

Prefer cat-heredoc to echo-complex-stmt.

1e663dc

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

Remove bash pipelines.

dfa3a10

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

Update a test name.

436475c

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

lint fix

cd19125

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

jandubois reviewed Aug 2, 2023

View reviewed changes

jandubois approved these changes Aug 2, 2023

View reviewed changes

jandubois mentioned this pull request Aug 2, 2023

Why do a shutdown during BATS teardown on macOS? #5274

Open

jandubois merged commit c728928 into main Aug 2, 2023
13 checks passed

jandubois deleted the 5020-fake-rdctl-convert-command branch August 2, 2023 01:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `rdctl create-profile [ --input FILE | --body JSON | --from-settings ]` #5021

Implement `rdctl create-profile [ --input FILE | --body JSON | --from-settings ]` #5021

ericpromislow commented Jun 21, 2023 •

edited

Loading

jandubois left a comment

ericpromislow commented Jun 22, 2023

jandubois commented Jun 22, 2023

jandubois left a comment

jandubois left a comment

jandubois Jul 28, 2023

ericpromislow Jul 28, 2023

github-advanced-security bot left a comment

jandubois left a comment

jandubois Jul 28, 2023

jandubois Jul 28, 2023

jandubois Aug 2, 2023

		# Just match a few of the lines near the start and the end of the output.
		# The unit tests do more comprehensive output checking.

	return fmt.Errorf("registry hive and type can't be specified with plist")
	return fmt.Errorf(`registry hive and type can't be specified with output "plist"`)

	return fmt.Errorf("invalid registry type of '%s' specified, must be %s or %s", outputSettingsFlags.RegistryProfileType, defaultsType, lockedType)
	return fmt.Errorf(`invalid registry type of "%s" specified, must be "%s" or "%s"`, outputSettingsFlags.RegistryProfileType, defaultsType, lockedType)

Implement rdctl create-profile [ --input FILE | --body JSON | --from-settings ] #5021

Implement rdctl create-profile [ --input FILE | --body JSON | --from-settings ] #5021

Conversation

ericpromislow commented Jun 21, 2023 • edited Loading

jandubois left a comment

Choose a reason for hiding this comment

ericpromislow commented Jun 22, 2023

jandubois commented Jun 22, 2023

jandubois left a comment

Choose a reason for hiding this comment

jandubois left a comment

Choose a reason for hiding this comment

jandubois Jul 28, 2023

Choose a reason for hiding this comment

ericpromislow Jul 28, 2023

Choose a reason for hiding this comment

github-advanced-security bot left a comment

Choose a reason for hiding this comment

jandubois left a comment

Choose a reason for hiding this comment

jandubois Jul 28, 2023

Choose a reason for hiding this comment

jandubois Jul 28, 2023

Choose a reason for hiding this comment

jandubois Aug 2, 2023

Choose a reason for hiding this comment

Implement `rdctl create-profile [ --input FILE | --body JSON | --from-settings ]` #5021

Implement `rdctl create-profile [ --input FILE | --body JSON | --from-settings ]` #5021

ericpromislow commented Jun 21, 2023 •

edited

Loading