Skip to content

blockstorage: add isPublic query option for volume types#3451

Merged
kayrus merged 2 commits intogophercloud:mainfrom
VoigtS:main
Jul 9, 2025
Merged

blockstorage: add isPublic query option for volume types#3451
kayrus merged 2 commits intogophercloud:mainfrom
VoigtS:main

Conversation

@VoigtS
Copy link
Copy Markdown
Contributor

@VoigtS VoigtS commented Jul 8, 2025

Fixes #3450

Links to the line numbers/files in the OpenStack source code that support the
code in this PR:
https://docs.openstack.org/api-ref/block-storage/v3/index.html#volume-types-types

@VoigtS VoigtS changed the title cinder: add isPublic query option for volume types blockstorage: add isPublic query option for volume types Jul 8, 2025
@github-actions github-actions bot added the edit:blockstorage This PR updates blockstorage code label Jul 8, 2025
Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for submitting your first PR! Be sure that we will be looking at it but keep in mind
this sometimes takes a while.
Please let the maintainers know if your PR has not got enough attention after a few days.
If any doubt, please consult our PR tutorial.

@github-actions github-actions bot added semver:minor Backwards-compatible change backport-v2 This PR will be backported to v2 labels Jul 8, 2025
@coveralls
Copy link
Copy Markdown

coveralls commented Jul 8, 2025

Coverage Status

coverage: 63.725% (+0.02%) from 63.702%
when pulling 181ac5c on VoigtS:main
into 7ed51ce on gophercloud:main.

type ListOpts struct {
// Specifies whether to request public or private types.
// Set to "none" to retrieve both types. Defaults to public only.
IsPublic string `q:"is_public"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this must be bool. if it defaults to true by default, then it must be a bool pointer. would be nice to have a unit test, see

IsPublic *bool `q:"is_public"`
and
th.TestFormValues(t, r, map[string]string{"all_tenants": "true"})
for example

Copy link
Copy Markdown
Contributor Author

@VoigtS VoigtS Jul 9, 2025

Choose a reason for hiding this comment

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

Thank you for the hint. Making this parameter a bool pointer is a good idea. However, when its value is nil, the BuildQueryString function ignores this parameter. This query is used for every OpenStack API implementation.

Without modifying this function and potentially causing side effects, translating this to the Python type None would require manually modifying the query string itself, which is an ugly hack. Therefore, I opted to create a string enum that can be used at the caller side to limit the query results. I'm open to discussing the naming of the variables (I don't think they are quite optimal yet; for example, if IsPublic has the value VisibilityPublic).

Another change is that now, by default, both public and private volume types are queried (when no ListOpts struct is provided). This aligns with the OpenStack client, which uses the parameter ?is_public=None by default. This can be verified by enabling the debug functionality of the client for the command: volume type list.

@github-actions github-actions bot added semver:major Breaking change and removed semver:minor Backwards-compatible change backport-v2 This PR will be backported to v2 labels Jul 9, 2025
Copy link
Copy Markdown
Contributor

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

VoigtS added 2 commits July 9, 2025 17:47
Adjust IsPublic type to utilize a string enum
Add test cases
@github-actions github-actions bot added semver:minor Backwards-compatible change backport-v2 This PR will be backported to v2 and removed semver:major Breaking change labels Jul 9, 2025
@kayrus kayrus merged commit 81cd18b into gophercloud:main Jul 9, 2025
18 checks passed
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jul 9, 2025

Failed to backport PR to v2 branch. See logs for details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-v2 This PR will be backported to v2 edit:blockstorage This PR updates blockstorage code semver:minor Backwards-compatible change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

blockstorage: IsPublic list option is missing

3 participants