Skip to content

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

Closed
pierreprinetti wants to merge 1 commit intogophercloud:masterfrom
shiftstack:secgroup_listoptsbuilder
Closed

networks: Security groups and sg rules List to accept a Builder#3070
pierreprinetti wants to merge 1 commit intogophercloud:masterfrom
shiftstack:secgroup_listoptsbuilder

Conversation

@pierreprinetti
Copy link
Copy Markdown
Member

@pierreprinetti pierreprinetti commented Jun 14, 2024

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
Copy Markdown

coveralls commented Jun 14, 2024

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

pierreprinetti commented Jun 23, 2024

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

@kayrus
Copy link
Copy Markdown
Contributor

kayrus commented Jul 10, 2024

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

@pierreprinetti
Copy link
Copy Markdown
Member Author

pierreprinetti commented Jul 10, 2024

Perfect, thanks @kayrus !

@pierreprinetti pierreprinetti deleted the secgroup_listoptsbuilder branch July 10, 2024 14:25
@kayrus
Copy link
Copy Markdown
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