Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

networks: Security groups and sg rules List to accept a Builder #3070

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
Closed
wants to merge 1 commit into from
Closed

networks: Security groups and sg rules List to accept a Builder #3070

wants to merge 1 commit into from

Conversation

pierreprinetti
Copy link
Member

Allow clients to pass a custom ListOptsBuilder to the List functions for both Security groups and Security group rules.

@pierreprinetti
networks: Security groups and sg rules List to accept a Builder
42096ea 
Allow clients to pass a custom ListOptsBuilder to the List functions for
both Security groups and Security group rules.
@github-actions github-actions bot added the semver:major Breaking change label Jun 14, 2024
@coveralls
Copy link

Coverage Status

coverage: 78.68% (-0.002%) from 78.682%
when pulling 42096ea on shiftstack:secgroup_listoptsbuilder
into 3bb883c on gophercloud:master.

@pierreprinetti
Copy link
Member Author

pierreprinetti commented Jun 14, 2024 •
edited
Loading

@mandre @jtopjian can you think of a reason why we didn't have that already?

@pierreprinetti pierreprinetti mentioned this pull request Jun 14, 2024
Merged
@pierreprinetti pierreprinetti requested a review from a team June 20, 2024 08:40
@mandre
Copy link
Contributor

mandre commented Jun 20, 2024

@mandre @jtopjian can you think of a reason why we didn't have that already?

If I were to guess, I'd say "no particular reason" other than we missed it at the time and no one noticed since.

There are 2 more we'll probably want to update while we're at it:

❯ ag "func List.+ListOpts\)" openstack
openstack/identity/v2/tenants/requests.go
20:func List(client *gophercloud.ServiceClient, opts *ListOpts) pagination.Pager {

openstack/networking/v2/extensions/layer3/routers/requests.go
40:func List(c *gophercloud.ServiceClient, opts ListOpts) pagination.Pager {

openstack/networking/v2/extensions/security/rules/requests.go
37:func List(c *gophercloud.ServiceClient, opts ListOpts) pagination.Pager {

openstack/networking/v2/extensions/security/groups/requests.go
34:func List(c *gophercloud.ServiceClient, opts ListOpts) pagination.Pager {

We have the same problem with Create and Update as well, and probably many other public functions:

❯ ag "func Create.+CreateOpts\)" openstack
openstack/blockstorage/v2/transfers/requests.go
26:func Create(ctx context.Context, client *gophercloud.ServiceClient, opts CreateOpts) (r CreateResult) {

openstack/blockstorage/v3/transfers/requests.go
26:func Create(ctx context.Context, client *gophercloud.ServiceClient, opts CreateOpts) (r CreateResult) {

openstack/compute/v2/aggregates/requests.go
34:func Create(ctx context.Context, client *gophercloud.ServiceClient, opts CreateOpts) (r CreateResult) {

openstack/identity/v3/ec2credentials/requests.go
37:func Create(ctx context.Context, client *gophercloud.ServiceClient, userID string, opts CreateOpts) (r CreateResult) {

openstack/networking/v2/extensions/bgp/peers/requests.go
46:func Create(ctx context.Context, c *gophercloud.ServiceClient, opts CreateOpts) (r CreateResult) {

openstack/networking/v2/extensions/bgp/speakers/requests.go
46:func Create(ctx context.Context, c *gophercloud.ServiceClient, opts CreateOpts) (r CreateResult) {

❯ ag "func Update.+UpdateOpts\)" openstack
openstack/baremetal/v1/ports/requests.go
202:func Update(ctx context.Context, client *gophercloud.ServiceClient, id string, opts UpdateOpts) (r UpdateResult) {

openstack/baremetal/v1/nodes/requests.go
328:func Update(ctx context.Context, client *gophercloud.ServiceClient, id string, opts UpdateOpts) (r UpdateResult) {

openstack/compute/v2/aggregates/requests.go
83:func Update(ctx context.Context, client *gophercloud.ServiceClient, aggregateID int, opts UpdateOpts) (r UpdateResult) {

openstack/compute/v2/services/requests.go
73:func Update(ctx context.Context, client *gophercloud.ServiceClient, id string, opts UpdateOpts) (r UpdateResult) {

openstack/loadbalancer/v2/flavorprofiles/requests.go
120:func Update(ctx context.Context, c *gophercloud.ServiceClient, id string, opts UpdateOpts) (r UpdateResult) {

openstack/loadbalancer/v2/flavors/requests.go
124:func Update(ctx context.Context, c *gophercloud.ServiceClient, id string, opts UpdateOpts) (r UpdateResult) {

openstack/loadbalancer/v2/listeners/requests.go
281:func Update(ctx context.Context, c *gophercloud.ServiceClient, id string, opts UpdateOpts) (r UpdateResult) {

openstack/loadbalancer/v2/loadbalancers/requests.go
211:func Update(ctx context.Context, c *gophercloud.ServiceClient, id string, opts UpdateOpts) (r UpdateResult) {

openstack/networking/v2/extensions/bgp/peers/requests.go
82:func Update(ctx context.Context, c *gophercloud.ServiceClient, bgpPeerID string, opts UpdateOpts) (r UpdateResult) {

We may consider implement a pre-submit check to make sure we don't introduce any more of them.

@pierreprinetti
Copy link
Member Author

@mandre Nice suggestions, thank you. This all looks like a good candidate for v3

@kayrus
Copy link
Contributor

kayrus commented Jul 10, 2024

@pierreprinetti I guess I unintendedly fixed the security groups listOpts builder in the #3092

@pierreprinetti
Copy link
Member Author

Perfect, thanks @kayrus !

@pierreprinetti pierreprinetti deleted the secgroup_listoptsbuilder branch July 10, 2024 14:25
@kayrus
Copy link
Contributor

kayrus commented Jul 10, 2024

@pierreprinetti only for groups :) I haven't fixed rule list

@kayrus kayrus mentioned this pull request Jul 10, 2024
Merged
@kayrus kayrus mentioned this pull request May 23, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
semver:major Breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pierreprinetti @coveralls @mandre @kayrus
Morty Proxy This is a proxified and sanitized view of the page, visit original site.