Settings handlers #2352

ricab · 2021-11-23T17:41:31Z

This PR implements settings handlers.

Start moving and adapting tests for functionality that was previously in the Settings class and is now located in the PersistentSettingsHandler.

Add a test to confirm the PersistentSettingsHandler returns the default value for a setting by default.

Fork an UnrecognizedSettingException from InvalidSettingsException.

Make it singular: InvalidSettingException.

Register a `PersistentSettingsHandler` to deal with client settings, in both the CLI and the GUI clients.

Register a temporary PersistentSettingsHandler for daemon settings in the client, in order to keep feature parity until we introduce routing handlers.

Temporarily derive path for daemon settings in client, to keep feature parity until we introduce routing handlers.

Transfer comment from old to new filename deriving functions, to preserve the info therein.

To allow mocking.

To be coherent with client side.

Generalize test file to test both client- and daemon-registered settings handlers.

Fixes constant getting garbage when compiled with MSVC.

Saviq

Kinda fun that git decided mock_settings.cpp is now basic_settings_spec.cpp 🤷.

First pass through roughly a half of this :)

include/multipass/settings/basic_setting_spec.h

include/multipass/settings/persistent_settings_handler.h

include/multipass/settings/settings.h

include/multipass/constants.h

src/client/cli/cmd/remote_settings_handler.cpp

include/multipass/settings/dynamic_setting_spec.h

src/daemon/daemon_init_settings.cpp

src/platform/platform_linux.cpp

src/settings/CMakeLists.txt

ricab · 2022-03-28T16:51:38Z

OK, finally circling back to this :)

Kinda fun that git decided mock_settings.cpp is now basic_settings_spec.cpp shrug.

I know! I was also surprised, but I guess the difference between them was smaller than a threshold. I bet there's a way to tell git otherwise...

It is not a default at all. Addresses review request.

Declare compilation units for the tests executable in alphabetical order. The order was already questionable in places, but got worse with new test files.

Saviq

Hey, just going through the tests now but wanted to register what I have. Tiny bits.

And kudos on this, it's a big one!

src/client/cli/cmd/remote_settings_handler.cpp

src/daemon/daemon_init_settings.cpp

src/daemon/daemon_main.cpp

src/settings/bool_setting_spec.cpp

tests/test_cli_client.cpp

tests/test_daemon.cpp

Saviq

Last thing is, this needs a rebase / merge :)

Other than that - works well, too :)

Applies review suggestions. Co-authored-by: Michał Sawicz <michal.sawicz@canonical.com>

As per review request.

Saviq

Yup. Will review the other side and merge first thing tomorrow.

src/daemon/daemon_main.cpp

Saviq

Found a problem with the passphrase handling. When you multipass set local.passphrase="", we need the stored passphrase to actually be blank:

# current
$ multipass version
multipass   1.10.0-dev.591.ci2957+g446817e7.mac
multipassd  1.10.0-dev.591.ci2957+g446817e7.mac

$ sudo multipass set local.passphrase=""

$ sudo multipass get local.passphrase
false

$ sudo cat /var/root/Library/Preferences/multipassd/multipassd.conf
[General]
local.passphrase=

# update
$ sudo installer -target / -dumplog -verbose -pkg multipass-1.10.0-dev.886.pr2352+g17e3ebc8.mac-Darwin.pkg

$ sudo multipass get local.passphrase
false

# but then
$ multipass set local.passphrase=""
$ multipass get local.passphrase
true

$ sudo cat /var/root/Library/Preferences/multipassd/multipassd.conf
[General]
local.passphrase=d72c87d0f077c7766f2985dfab30e8955c373a13a1e93d315203939f542ff86e73ee37c31f4c4b571f4719fa8e3589f12db8dcb57ea9f56764bb7d58f64cf705

ricab · 2022-04-07T09:09:34Z

Hmm good catch. Will dig.

Check that the input password is hashed and reset.

Saviq

Yup!

All good now :)

bors merge

(I know you won't :P)

bors · 2022-04-07T13:17:46Z

Build failed:

Windows

2352: Settings handlers r=Saviq a=ricab This PR implements settings handlers. Co-authored-by: Ricardo Abreu <ricardo.abreu@canonical.com>

ricab added 30 commits November 15, 2021 18:14

[settings] Replicate keys logic in std handler

c7c9c67

[settings] Slight class and header reorganization

1686081

[test] Start moving/adapting tests to handler

f512f9f

Start moving and adapting tests for functionality that was previously in the Settings class and is now located in the PersistentSettingsHandler.

[settings] Fix file name for persistent settings

ead2299

[test] Move and adapt more tests to handler

0615b54

[test] Continue moving/adapting tests to handler

a871809

[test] Check persistent settings defaults

cb16003

Add a test to confirm the PersistentSettingsHandler returns the default value for a setting by default.

[test] Split InvalidSettingsException in two

89f50e1

Fork an UnrecognizedSettingException from InvalidSettingsException.

[settings] Rename InvalidSettingsException

0bacb9a

Make it singular: InvalidSettingException.

[test] Refactor common bits out of handler tests

49a282b

[test] Check handling of unknown settings key

9606c13

[test] Check that handler writes to given filename

709da9b

[client] Register handler for client settings

b296c13

Register a `PersistentSettingsHandler` to deal with client settings, in both the CLI and the GUI clients.

[client] Register tmp handler for daemon settings

7e7de2f

Register a temporary PersistentSettingsHandler for daemon settings in the client, in order to keep feature parity until we introduce routing handlers.

[client] Derive path for client settings

7fa9c22

[client] Derive path for daemon settings (temp)

870dd1a

Temporarily derive path for daemon settings in client, to keep feature parity until we introduce routing handlers.

[client] Preserve info in comment

7602e87

Transfer comment from old to new filename deriving functions, to preserve the info therein.

[test] Start testing clients' registered handlers

8574b64

[test] Extract MockQSettings into own header

dbce607

[test] Check default settings in client handler

4715757

[platform] Move extra settings to singleton

e85e087

To allow mocking.

[test] Check platform extras in client handler

937a156

[test] Filename in clients' persistent handler

c2601ff

[daemon] Repurpose files for generic settings init

192e98b

[daemon] Move settings init to daemon namespace

7ee1b5b

To be coherent with client side.

[daemon] Register persistent handler for daemon

3ad1428

[test] Generalize test file for client/daemon

2fa2814

Generalize test file to test both client- and daemon-registered settings handlers.

[test] Factor out grabbing of registered handler

8caf9c4

[platform] Move daemon config home to singleton

a84b78b

[test] Filename in daemon's persistent handler

0f00726

ricab force-pushed the settings-handlers branch from 71b741b to 66a44a7 Compare March 8, 2022 15:25

[test] Add missing includes

f516881

ricab force-pushed the settings-handlers branch from 66a44a7 to f516881 Compare March 8, 2022 20:00

ricab added 3 commits March 9, 2022 19:04

[test] Drop constexpr from initializer_list const

e89ce48

Fixes constant getting garbage when compiled with MSVC.

[test] Fix comparison of std and c strings on win

f28730b

[doc] Add TODO to move prompters into settings

c6ad110

Saviq requested changes Mar 22, 2022

View reviewed changes

ricab added 5 commits March 28, 2022 19:36

[networks] Revert wrong constant name

45efcab

It is not a default at all. Addresses review request.

[settings] Rename {Dynamic,Custom}SettingSpec

d214952

Merge branch 'main' into settings-handlers

f5d890e

[tests] Fix bad file ordering in cmake declaration

7e5c3fb

Declare compilation units for the tests executable in alphabetical order. The order was already questionable in places, but got worse with new test files.

Merge branch 'main' into settings-handlers

3c69c8f

Saviq reviewed Apr 6, 2022

View reviewed changes

Saviq requested changes Apr 6, 2022

View reviewed changes

ricab and others added 2 commits April 6, 2022 17:51

[settings] Improve error messages

a02b0b4

Applies review suggestions. Co-authored-by: Michał Sawicz <michal.sawicz@canonical.com>

[daemon] Add TODOs on settings monitoring

47688d9

As per review request.

ricab force-pushed the settings-handlers branch from 08d41d1 to 47688d9 Compare April 6, 2022 17:30

Saviq approved these changes Apr 6, 2022

View reviewed changes

src/daemon/daemon_main.cpp Show resolved Hide resolved

Saviq requested changes Apr 7, 2022

View reviewed changes

ricab added 2 commits April 7, 2022 11:26

[daemon] Fix password resetting

4ed6ae7

[tests] Check custom password handler

481e96b

Check that the input password is hashed and reset.

Saviq approved these changes Apr 7, 2022

View reviewed changes

townsend2010 merged commit 0a776a2 into main Apr 7, 2022

bors bot deleted the settings-handlers branch April 7, 2022 13:39

townsend2010 pushed a commit that referenced this pull request Apr 7, 2022

Merge #2352

4d1de54

2352: Settings handlers r=Saviq a=ricab This PR implements settings handlers. Co-authored-by: Ricardo Abreu <ricardo.abreu@canonical.com>

townsend2010 pushed a commit that referenced this pull request Apr 7, 2022

Merge #2352

c1a4c09

2352: Settings handlers r=Saviq a=ricab This PR implements settings handlers. Co-authored-by: Ricardo Abreu <ricardo.abreu@canonical.com>

townsend2010 pushed a commit that referenced this pull request Apr 7, 2022

Merge #2352

9766270

2352: Settings handlers r=Saviq a=ricab This PR implements settings handlers. Co-authored-by: Ricardo Abreu <ricardo.abreu@canonical.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Settings handlers #2352

Settings handlers #2352

ricab commented Nov 23, 2021

Saviq left a comment

ricab commented Mar 28, 2022

Saviq left a comment

Saviq left a comment

Saviq left a comment

Saviq left a comment

ricab commented Apr 7, 2022

Saviq left a comment

bors bot commented Apr 7, 2022

Settings handlers #2352

Settings handlers #2352

Conversation

ricab commented Nov 23, 2021

Saviq left a comment

Choose a reason for hiding this comment

ricab commented Mar 28, 2022

Saviq left a comment

Choose a reason for hiding this comment

Saviq left a comment

Choose a reason for hiding this comment

Saviq left a comment

Choose a reason for hiding this comment

Saviq left a comment

Choose a reason for hiding this comment

ricab commented Apr 7, 2022

Saviq left a comment

Choose a reason for hiding this comment

bors bot commented Apr 7, 2022