Fix/set attributes cmd #11961

gabrielmougard · 2023-07-07T10:12:22Z

closes #10960

monstermunchkin · 2023-07-07T14:00:41Z

@gabrielmougard could you please add some tests?

lxc/cluster.go

lxc/network.go

lxc/network_acl.go

lxc/storage_bucket.go

lxc/storage_volume.go

lxc/cluster.go

lxc/utils.go

ru-fu · 2023-07-11T08:58:51Z

Should this be property instead of attribute?
https://documentation.ubuntu.com/lxd/en/latest/explanation/instance_config/

We only have it explicitly documented for instances, but in the docs, we're trying to be consistent in calling everything under config "options"/"configuration options" and everything on top level "properties".

monstermunchkin · 2023-07-11T09:50:07Z

Should this be property instead of attribute? https://documentation.ubuntu.com/lxd/en/latest/explanation/instance_config/

Yes, we should call it property.

ru-fu · 2023-07-11T13:38:30Z

Thanks!
Can you add a "Configure instance properties" section to https://documentation.ubuntu.com/lxd/en/latest/howto/instances_configure/ and update https://documentation.ubuntu.com/lxd/en/latest/explanation/instance_config/ to link to that section for instance properties?

We should probably also update the other configuration how-tos, but we can do that later if needed.

tomponline

Please can we have some tests added for this functionality too please.

lxc/utils.go

gabrielmougard · 2023-07-11T17:04:52Z

@tomponline I added two tests (for a string property and for a bool property) but only for an instance object. As the code is the same for all the other entities (network, network_acl, network forward, profile, etc.) should I add the same tests for each entity ?

tomponline

Please can you use the normal i18n: Update translation templates message style (without typo ;) too) when updating i18n.

tomponline · 2023-07-14T09:57:14Z

@ru-fu I expect that we should update the docs for this new CLI feature, do you have a preference as to where?

tomponline · 2023-07-14T09:58:16Z

@ru-fu oh wait you already did in #11961 (comment), @gabrielmougard please can you add. thanks.

tomponline

Please can you split your Go commits by file so its possible to backport the ones that are supported by the LTS.

lxc/utils.go

tomponline · 2023-07-14T11:59:54Z

@gabrielmougard looks like this needs a rebase now.

tomponline

As discussed lets add a test for setting/getting the instance snapshot and storage volume snapshot expiry times.

tomponline · 2023-07-17T12:43:48Z

Needs a rebase

doc/explanation/instance_config.md

doc/howto/instances_configure.md

ru-fu · 2023-07-17T12:56:48Z

doc/howto/instances_configure.md

+To update instance properties after the instance is created, use the `lxc config set` command with the `--property` flag.
+Specify the instance name and the key and value of the instance property:
+
+    lxc config set <instance_name> <option_key>=<option_value> <option_key>=<option_value> ... --property


What happens if some of these properties are actually options?

Then the command will output an error saying that the <option_key> has not been found

Then the command will output an error saying that the <option_key> has not been found

How much effort would it be to check if the key exists as an option, and in that case report that back to the user?

We're making a difference between properties and options in the docs, but we cannot necessarily expect users to be aware of the difference. And I could imagine users getting quite confused if we're telling them that one of the keys, which they have used before, suddenly can't be found anymore.

doc/howto/instances_configure.md

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

…rties Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

test/suites/config.sh

ru-fu

Docs look good.

tomponline · 2023-07-18T12:35:22Z

static analysis failing

test/suites/config.sh

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

monstermunchkin suggested changes Jul 7, 2023

View reviewed changes

lxc/utils.go Outdated Show resolved Hide resolved

lxc/utils.go Outdated Show resolved Hide resolved

gabrielmougard force-pushed the fix/set-attributes-cmd branch from 18a7049 to bb455d9 Compare July 11, 2023 13:08

tomponline requested changes Jul 11, 2023

View reviewed changes

monstermunchkin suggested changes Jul 11, 2023

View reviewed changes

lxc/utils.go Outdated Show resolved Hide resolved

gabrielmougard force-pushed the fix/set-attributes-cmd branch from bb455d9 to b6d5c05 Compare July 11, 2023 17:00

gabrielmougard force-pushed the fix/set-attributes-cmd branch from b6d5c05 to 758b930 Compare July 11, 2023 17:09

gabrielmougard requested a review from tomponline July 11, 2023 17:10

tomponline requested changes Jul 12, 2023

View reviewed changes

gabrielmougard force-pushed the fix/set-attributes-cmd branch from 758b930 to c74b313 Compare July 12, 2023 14:10

tomponline requested changes Jul 14, 2023

View reviewed changes

tomponline reviewed Jul 14, 2023

View reviewed changes

lxc/utils.go Outdated Show resolved Hide resolved

tomponline reviewed Jul 14, 2023

View reviewed changes

lxc/utils.go Outdated Show resolved Hide resolved

gabrielmougard force-pushed the fix/set-attributes-cmd branch 2 times, most recently from 76f91ef to eb8255f Compare July 14, 2023 15:55

gabrielmougard requested a review from tomponline July 14, 2023 15:55

gabrielmougard force-pushed the fix/set-attributes-cmd branch from eb8255f to 01c39d9 Compare July 14, 2023 16:13

github-actions bot added the Documentation Documentation needs updating label Jul 14, 2023

tomponline requested a review from ru-fu July 15, 2023 07:50

tomponline requested changes Jul 17, 2023

View reviewed changes

ru-fu requested changes Jul 17, 2023

View reviewed changes

gabrielmougard force-pushed the fix/set-attributes-cmd branch 2 times, most recently from e646bcf to 32eb4ec Compare July 17, 2023 15:33

gabrielmougard added 15 commits July 17, 2023 17:34

lxc: Add stringTo{ Type }HookFunc utils func

6356ca0

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

test: Add unit tests for utils_properties.go

995a814

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

lxc: Update get/set/unset command for cluster properties

dcf3015

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

lxc: Update get/set/unset command for instance properties

a6d6d4b

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

lxc: Update get/set/unset command for network ACL properties

03fb831

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

lxc: Update get/set/unset command for network forward properties

d6a41d3

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

lxc: Update get/set/unset command for network load-balancer prope…

2dd1c1a

…rties Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

lxc: Update get/set/unset command for network peer properties

69affc4

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

lxc: Update get/set/unset command for network zone properties

a0cbef2

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

lxc: Update get/set/unset command for network properties

aeefded

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

lxc: Update get/set/unset command for profile properties

f5d7da5

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

lxc: Update get/set/unset command for project properties

e275ead

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

lxc: Update get/set/unset command for storage bucket properties

e140769

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

lxc: Update get/set/unset command for storage volume properties

7458608

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

lxc: Update get/set/unset command for storage properties

99e08ce

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

gabrielmougard force-pushed the fix/set-attributes-cmd branch from 32eb4ec to 9576afa Compare July 17, 2023 15:35

tomponline reviewed Jul 17, 2023

View reviewed changes

test/suites/config.sh Outdated Show resolved Hide resolved

gabrielmougard force-pushed the fix/set-attributes-cmd branch from 9576afa to 7bce333 Compare July 18, 2023 09:10

ru-fu approved these changes Jul 18, 2023

View reviewed changes

tomponline reviewed Jul 18, 2023

View reviewed changes

test/suites/config.sh Outdated Show resolved Hide resolved

gabrielmougard added 3 commits July 18, 2023 14:53

test: get/set/unset of instance properties

3c90a68

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

doc: Update the doc to describe how to get/set/unset instance properties

e4ef5c1

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

i18n: Update translation files

f2f8d41

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

gabrielmougard force-pushed the fix/set-attributes-cmd branch from 7bce333 to f2f8d41 Compare July 18, 2023 12:54

tomponline merged commit e5a365e into canonical:main Jul 18, 2023
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/set attributes cmd #11961

Fix/set attributes cmd #11961

gabrielmougard commented Jul 7, 2023

monstermunchkin commented Jul 7, 2023

ru-fu commented Jul 11, 2023 •

edited

monstermunchkin commented Jul 11, 2023

ru-fu commented Jul 11, 2023

tomponline left a comment

gabrielmougard commented Jul 11, 2023

tomponline left a comment

tomponline commented Jul 14, 2023

tomponline commented Jul 14, 2023

tomponline left a comment

tomponline commented Jul 14, 2023

tomponline left a comment

tomponline commented Jul 17, 2023

ru-fu Jul 17, 2023

gabrielmougard Jul 18, 2023

ru-fu Jul 18, 2023

ru-fu left a comment

tomponline commented Jul 18, 2023

Fix/set attributes cmd #11961

Fix/set attributes cmd #11961

Conversation

gabrielmougard commented Jul 7, 2023

monstermunchkin commented Jul 7, 2023

ru-fu commented Jul 11, 2023 • edited

monstermunchkin commented Jul 11, 2023

ru-fu commented Jul 11, 2023

tomponline left a comment

Choose a reason for hiding this comment

gabrielmougard commented Jul 11, 2023

tomponline left a comment

Choose a reason for hiding this comment

tomponline commented Jul 14, 2023

tomponline commented Jul 14, 2023

tomponline left a comment

Choose a reason for hiding this comment

tomponline commented Jul 14, 2023

tomponline left a comment

Choose a reason for hiding this comment

tomponline commented Jul 17, 2023

ru-fu Jul 17, 2023

Choose a reason for hiding this comment

gabrielmougard Jul 18, 2023

Choose a reason for hiding this comment

ru-fu Jul 18, 2023

Choose a reason for hiding this comment

ru-fu left a comment

Choose a reason for hiding this comment

tomponline commented Jul 18, 2023

ru-fu commented Jul 11, 2023 •

edited