Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prevent custom config keys being removed when updating resources [WD-9348] #691

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

mas-who
Copy link
Contributor

@mas-who mas-who commented Mar 8, 2024

Done

  • Checked edit instances (using getUnhandledKeyValues)
  • Checked edit profile (using getUnhandledKeyValues)
  • Checked edit network (using yaml object)
  • Checked edit network forward (no config keys)
  • Checked edit storage pool (using PATCH request so it's not a problem)
  • Checked edit storage volume (updated to use getUnhandledKeyValues)
  • Checked edit snapshot (no config keys)
  • Checked edit image (can't edit images so not a problem)
  • Checked edit project (using getUnhandledKeyValues)

QA

  1. Run the LXD-UI:
    • On the demo server via the link posted by @webteam-app below. This is only available for PRs created by collaborators of the repo. Ask @mas-who or @edlerd for access.
    • With a local copy of this branch, run as described here.
  2. Perform the following QA steps:
    • Unfortunately not all resource types have yaml support yet e.g. storage pool, storage volume, project etc, QA for this PR will be a bit difficult on the demo server, recommend testing this locally.
    • For each resource type (e.g. instance, network, profile etc), create a new instance of that resource type. Using the lxd cli, set a custom config key for that resource with lxc [resource-type] set [resource-name] user.foo=bar, then check its configs using lxc [resource-type] show [resource-name] and confirm user.foo is present in the resource configs. From there, navigate to the edit page for that resource type, modify anything and then hit save. Run lxc [resource-type] show [resource-name] again and confirm that user.foo is still present in the configs. NOTE: the lxc command for setting and showing resource configs can be slightly different depending on the resource type, please check the MAN page relating to each resource type's cli commands.

@webteam-app
Copy link

Demo starting at https://lxd-ui-691.demos.haus

@mas-who mas-who changed the title fix: prevent custom config keys being removed when updating resources fix: prevent custom config keys being removed when updating resources [WD-9348] Mar 8, 2024
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA looks good, we should also handle unknown main keys (not only the nested ones under config). Added an idea for making the code a tiny bit simpler.

@mas-who mas-who force-pushed the persist-custom-keys branch 2 times, most recently from 7c781f7 to f0e5ec4 Compare March 11, 2024 16:55
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for checking and fixing this!

- Checked edit instances (using getUnhandledKeyValues)
- Checked edit profile (using getUnhandledKeyValues)
- Checked edit network (using yaml object)
- Checked edit network forward (no config keys)
- Checked edit storage pool (using PATCH request so it's not a problem)
- Checked edit storage volume (updated to use getUnhandledKeyValues)
- Checked edit snapshot (no config keys)
- Checked edit image (can't edit images so not a problem)
- Checked edit project (using getUnhandledKeyValues)

Signed-off-by: Mason Hu <mason.hu@canonical.com>
@mas-who mas-who merged commit 0cd7e58 into canonical:main Mar 11, 2024
10 checks passed
github-actions bot pushed a commit that referenced this pull request Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants