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

feat: [WD-18264] CMS fields for storage pool source #1070

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

Kxiru
Copy link
Contributor

@Kxiru Kxiru commented Jan 22, 2025

Done

  • Added intermediary StoragePoolClusteredSourceSelector file + a ClusterSpecificInput component.
  • Added functionality to create storage pool source fields for specific cluster members.

QA

  1. Run the LXD-UI:
  2. Perform the following QA steps:
    • Within a clustered environment, navigate to the Create a storage pool page, and attempt to create a storage pool with a ZFS, LVM or Directory driver. (Directory is less prone to error and empty directories can be made for testing purposes by creating them in the terminal of the cluster node).
    • With a storage pool successfully created, navigate to it's configuration page and verify that
      • The page defaults to the correct setting (global source or single clusters sources)
      • The previously input sources persist.

Screenshots

image
image

@webteam-app
Copy link

@Kxiru Kxiru force-pushed the cms-fields-storage-pool-source branch 3 times, most recently from 6d79915 to b87a423 Compare January 24, 2025 03:24
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.

Good progress, some comments on the code below.

src/api/storage-pools.tsx Outdated Show resolved Hide resolved
src/pages/storage/forms/StoragePoolForm.tsx Show resolved Hide resolved
src/pages/storage/forms/StoragePoolFormMain.tsx Outdated Show resolved Hide resolved
src/util/storagePoolForm.tsx Show resolved Hide resolved
@Kxiru Kxiru requested a review from edlerd January 24, 2025 22:30
@Kxiru Kxiru force-pushed the cms-fields-storage-pool-source branch from b87a423 to 7e47745 Compare January 24, 2025 22:32
@Kxiru Kxiru marked this pull request as ready for review January 24, 2025 22:33
@Kxiru Kxiru force-pushed the cms-fields-storage-pool-source branch from 7e47745 to 173b015 Compare January 24, 2025 22:36
@Kxiru Kxiru changed the title feat: WD[18264] CMS fields for storage pool source feat: [WD-18264] CMS fields for storage pool source Jan 24, 2025
@Kxiru Kxiru force-pushed the cms-fields-storage-pool-source branch 3 times, most recently from 114dc76 to fb81463 Compare January 24, 2025 23:03
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.

It is fully functional now, good progress. I found some QA and code issues below.

src/pages/storage/EditStoragePool.tsx Outdated Show resolved Hide resolved
src/util/helpers.tsx Show resolved Hide resolved
src/components/forms/ClusterSpecificInput.tsx Outdated Show resolved Hide resolved
src/components/forms/ClusterSpecificInput.tsx Outdated Show resolved Hide resolved
src/components/forms/ClusterSpecificInput.tsx Outdated Show resolved Hide resolved
src/pages/storage/forms/StoragePoolFormMain.tsx Outdated Show resolved Hide resolved
src/pages/storage/forms/StoragePoolFormMain.tsx Outdated Show resolved Hide resolved
src/pages/storage/forms/StoragePoolFormMain.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@mas-who mas-who left a comment

Choose a reason for hiding this comment

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

Good work! Some code comments so far

src/api/storage-pools.tsx Outdated Show resolved Hide resolved
src/api/storage-pools.tsx Outdated Show resolved Hide resolved
src/pages/storage/forms/StoragePoolFormMain.tsx Outdated Show resolved Hide resolved
src/pages/storage/forms/StoragePoolFormMain.tsx Outdated Show resolved Hide resolved
src/pages/storage/forms/StoragePoolFormMain.tsx Outdated Show resolved Hide resolved
src/components/forms/ClusterSpecificInput.tsx Show resolved Hide resolved
@mas-who
Copy link
Collaborator

mas-who commented Jan 27, 2025

@Kxiru I realise some of my comments may have overlapped with comments from @edlerd . Please resolve the issues if it's already addressed by your responses to @edlerd

@Kxiru Kxiru force-pushed the cms-fields-storage-pool-source branch 3 times, most recently from bff8ec7 to 24da8cd Compare January 27, 2025 19:16
@Kxiru Kxiru requested review from mas-who and edlerd January 27, 2025 19:16
src/components/forms/ClusterSpecificInput.tsx Outdated Show resolved Hide resolved
src/components/forms/ClusterSpecificInput.tsx Outdated Show resolved Hide resolved
src/components/forms/ClusterSpecificInput.tsx Outdated Show resolved Hide resolved
src/components/forms/ClusterSpecificInput.tsx Outdated Show resolved Hide resolved
src/components/forms/ClusterSpecificInput.tsx Show resolved Hide resolved
@Kxiru Kxiru force-pushed the cms-fields-storage-pool-source branch from 24da8cd to 0db42e6 Compare January 28, 2025 14:23
@Kxiru Kxiru force-pushed the cms-fields-storage-pool-source branch from 0db42e6 to b6d3346 Compare January 28, 2025 14:24
@Kxiru Kxiru requested review from edlerd and mas-who January 28, 2025 14:26
@Kxiru Kxiru force-pushed the cms-fields-storage-pool-source branch from b6d3346 to 280bdd9 Compare January 28, 2025 14:50
@Kxiru Kxiru force-pushed the cms-fields-storage-pool-source branch from 280bdd9 to 151aa39 Compare January 28, 2025 15:28
@Kxiru Kxiru force-pushed the cms-fields-storage-pool-source branch from 151aa39 to 1a302b3 Compare January 28, 2025 15:52
@Kxiru Kxiru requested a review from edlerd January 28, 2025 15:52
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 and code LGTM, tiny non-blocking nitpick on the class naming, good to go as is.

src/sass/_cluster_specific_input.scss Outdated Show resolved Hide resolved
edlerd
edlerd previously approved these changes Jan 28, 2025
@Kxiru Kxiru force-pushed the cms-fields-storage-pool-source branch from 8c4c54c to d85e792 Compare January 28, 2025 16:47
@Kxiru Kxiru merged commit 029ccc8 into canonical:main Jan 28, 2025
11 checks passed
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.

4 participants