Skip to content

nova: introduce servers.ListSimple along with the more detailed servers.List#2555

Merged
EmilienM merged 1 commit intogophercloud:masterfrom
kayrus:servers-list-detail
Feb 13, 2023
Merged

nova: introduce servers.ListSimple along with the more detailed servers.List#2555
EmilienM merged 1 commit intogophercloud:masterfrom
kayrus:servers-list-detail

Conversation

@kayrus
Copy link
Copy Markdown
Contributor

@kayrus kayrus commented Feb 8, 2023

Fixes #2553

@coveralls
Copy link
Copy Markdown

coveralls commented Feb 8, 2023

Coverage Status

Coverage: 80.096% (+0.0007%) from 80.096% when pulling ad4f2f0 on kayrus:servers-list-detail into 49ee5b4 on gophercloud:master.

@pierreprinetti
Copy link
Copy Markdown
Member

Can you think of a way of adding this functionality in a non-breaking fashion?

@kayrus
Copy link
Copy Markdown
Contributor Author

kayrus commented Feb 8, 2023

@pierreprinetti I'd like to, but it won't be aligned with other services... See my thoughts in the original ticket.

@pierreprinetti
Copy link
Copy Markdown
Member

I don't think it's reasonable to merge a fix that breaks existing users for efficiency. If it's not possible to achieve what you need in a backwards-compatible manner, I'd say it's a wontfix.

@kayrus
Copy link
Copy Markdown
Contributor Author

kayrus commented Feb 9, 2023

I know it's a breaking change. But it's breaks for good reasons: align with other ListDetails functions in a project:

$ grep -rl 'func ListDetail(' openstack/
openstack/compute/v2/servers/requests.go
openstack/compute/v2/images/requests.go
openstack/compute/v2/flavors/requests.go
openstack/compute/v2/extensions/availabilityzones/requests.go
openstack/sharedfilesystems/v2/schedulerstats/requests.go
openstack/sharedfilesystems/v2/sharenetworks/requests.go
openstack/sharedfilesystems/v2/shares/requests.go
openstack/sharedfilesystems/v2/snapshots/requests.go
openstack/baremetal/v1/ports/requests.go
openstack/baremetal/v1/nodes/requests.go
openstack/blockstorage/extensions/backups/requests.go
openstack/containerinfra/v1/clusters/requests.go

It's not the first time gophercloud introduces breaking changes, see https://github.com/gophercloud/gophercloud/blob/master/CHANGELOG.md

@pierreprinetti
Copy link
Copy Markdown
Member

It's not the first time gophercloud introduces breaking changes

I am not against breaking changes. However they have a cost. Increased efficiency is rarely going to balance that cost.

@kayrus
Copy link
Copy Markdown
Contributor Author

kayrus commented Feb 9, 2023

@pierreprinetti ok. I appreciate if you can suggest a name for a simple list function.

@pierreprinetti
Copy link
Copy Markdown
Member

ehr... ListSimple?

I hope you are more creative than I am 😬

@kayrus kayrus force-pushed the servers-list-detail branch from bc0070e to ad4f2f0 Compare February 9, 2023 14:39
@kayrus
Copy link
Copy Markdown
Contributor Author

kayrus commented Feb 9, 2023

@pierreprinetti done

@pierreprinetti
Copy link
Copy Markdown
Member

Thank you for being so quick implementing the ugly name I came up with 😝

@mandre @EmilienM WDYT?

@EmilienM EmilienM merged commit c4fd26e into gophercloud:master Feb 13, 2023
@kayrus kayrus deleted the servers-list-detail branch February 13, 2023 14:19
@mandre mandre added this to the v1.3.0 milestone Feb 23, 2023
@mandre mandre changed the title nova: introduce servers.ListDetails along with a simple servers.List nova: introduce servers.ListSimple along with the more detailed servers.List Mar 1, 2023
@mandre
Copy link
Copy Markdown
Contributor

mandre commented Mar 1, 2023

I renamed the PR to reflect the latest code change.

@pierreprinetti
Copy link
Copy Markdown
Member

We merged this a bit too fast. List and ListSimple should be better documented (they currently have the same godoc) and both List and ListSimple should have tests that prove their different behaviour.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nova: List request always uses detailed server listing

5 participants