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.

Sign up for GitHub

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

Jump to bottom

API: Add list all volumes endpoint #13036

Merged
merged 13 commits into from
Mar 27, 2024
Merged

API: Add list all volumes endpoint #13036

merged 13 commits into from
Mar 27, 2024

Conversation

hamistao
Copy link
Contributor

@hamistao hamistao commented Mar 4, 2024 •
edited by tomponline
Loading

Added new endpoint /1.0/storage_volumes, and made some changes made necessary by the addition of the Pool field on api.StorageVolume.
Fixes #12550.

@github-actions github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Mar 4, 2024
@hamistao hamistao force-pushed the issue12550/add_list_all_volumes_endpoint branch from 003ea5e to d4343f9 Compare March 4, 2024 16:02
@tomponline tomponline changed the title Issue12550/add list all volumes endpoint API: Add list all volumes endpoint Mar 4, 2024
@tomponline
Copy link
Member

It looks like you need to rebase to lose the ceph commit thats been merged.

@tomponline
Copy link
Member

Please can you ensure the API extension commit is first in the PR. This helps with backporting the change ta.

@hamistao
Copy link
Contributor Author

hamistao commented Mar 6, 2024

I decided to add a pool column when listing all volumes.

@hamistao hamistao force-pushed the issue12550/add_list_all_volumes_endpoint branch 17 times, most recently from 1c908e3 to c673ae9 Compare March 7, 2024 21:59
@hamistao hamistao marked this pull request as ready for review March 7, 2024 22:10
@hamistao hamistao requested a review from tomponline as a code owner March 7, 2024 22:10
@hamistao
Copy link
Contributor Author

hamistao commented Mar 7, 2024

I decided to add a Pool column when listing all volumes with the CLI, if it is unecessary I can remove it.

@hamistao
Copy link
Contributor Author

hamistao commented Mar 7, 2024

Also I took liberty to add the all-projects field to the existing endpoint for listing volumes on swaggerdoc since it was such a minor change. If this is a problem just let me know.

@hamistao hamistao force-pushed the issue12550/add_list_all_volumes_endpoint branch from c673ae9 to 6ebe3c7 Compare March 8, 2024 03:40
tomponline
tomponline reviewed Mar 20, 2024
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

@markylaing please can you advise @hamistao if/where we need a an authorizer check when listing storage volumes?

@tomponline tomponline requested a review from markylaing March 20, 2024 09:42
markylaing
markylaing previously requested changes Mar 20, 2024
View reviewed changes
Copy link
Contributor

@markylaing markylaing left a comment

Choose a reason for hiding this comment

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

Overall this looks great. I've added a couple of small points and I think it's worth a discussion about whether the queries for all volumes should be optimised.

doc/api-extensions.md Outdated Show resolved Hide resolved
lxd/storage_volumes.go Show resolved Hide resolved
lxd/storage_volumes.go Show resolved Hide resolved
lxc/storage_volume.go Outdated Show resolved Hide resolved
lxc/storage_volume.go Outdated Show resolved Hide resolved
@markylaing
Copy link
Contributor

@markylaing please can you advise @hamistao if/where we need a an authorizer check when listing storage volumes?

As the authorization filtering is occurring at the end of the handler I don't think any changes need to be made here. However, it would be good to prove that the filtering works by adding a test for this - perhaps in test/suites/authorization.sh

@hamistao hamistao force-pushed the issue12550/add_list_all_volumes_endpoint branch 4 times, most recently from 289f8a9 to c732f66 Compare March 25, 2024 12:51
@hamistao
Copy link
Contributor Author

hamistao commented Mar 25, 2024 •
edited
Loading

Not sure this is enough, would you mind giving me your opinion @markylaing?
https://github.com/hamistao/lxd/blob/issue12550/add_list_all_volumes_endpoint/test/suites/auth.sh#L354-L355
Also I assumed by test/suites/authorization.sh you meant test/suites/auth.sh

@tomponline
Copy link
Member

Please can you rebase and solve conflicts

hamistao added 13 commits March 27, 2024 11:12
@hamistao
api: Add storage_volumes_all extension
b6eae3f 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
shared/api: Add Pool field to api.StorageVolume
b1995d1 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
lxd: Remove uncecessary parameter from URL function
9d16af4 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
shared/api: Update call to URL function
5ccc0e4 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
lxd: Remove uncecessary parameter from storagePoolVolumeUsedByGet
a9c7f9e 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
lxd: Update storagePoolVolumeUsedByGet usage
835c42b 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
lxd/db: Update get volume query
ebd86ea 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
lxd: Add endpoints to list all volumes
8fef330 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
client: Add functions to get all volumes
eede575 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
lxc/storage_volume.go: Update lxc storage volume list
1067424 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
test: Add tests for listing volumes from all pools
d75b1ab 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
i18n: Update translations
0a5d8b8 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
doc: Run make update-api
ae3176d 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao hamistao force-pushed the issue12550/add_list_all_volumes_endpoint branch from c732f66 to ae3176d Compare March 27, 2024 14:12
@github-actions GitHub Actions
Copy link

Heads up @ru-fu - the "Documentation" label was applied to this issue.

@hamistao
Copy link
Contributor Author

Please can you rebase and solve conflicts
@tomponline done

tomponline
tomponline approved these changes Mar 27, 2024
View reviewed changes
Copy link
Member

@tomponline tomponline 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!

@tomponline tomponline merged commit e6f9af1 into canonical:main Mar 27, 2024
29 checks passed
@hamistao hamistao deleted the issue12550/add_list_all_volumes_endpoint branch June 6, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MusicDin MusicDin MusicDin left review comments

@tomponline tomponline tomponline approved these changes

@markylaing markylaing markylaing left review comments

Assignees
No one assigned
Labels
API Changes to the REST API Documentation Documentation needs updating
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

List all storage volumes via API
4 participants
@hamistao @tomponline @markylaing @MusicDin