Fix list ports with multiple fixedip parameters#2806
Fix list ports with multiple fixedip parameters#2806pierreprinetti merged 2 commits intogophercloud:masterfrom
Conversation
|
I'm anticipating that "Subnet ID and IP Address" will fail. If it does I'll open an issue. |
6d321fd to
6a7ae90
Compare
|
Test failed successfully 😁 |
|
Semver arguments: It doesn't affect semver at all because it's a bugfix. It previously did not work, now it works. A user using gophercloud correctly would previously get no returned ports, where now they get returned ports. This is a breaking change in the previous behaviour of the gophercloud API, so is semver:major. While the latter may be technically correct, I believe the former is more useful: it's just a bugfix. |
|
/lgtm |
|
WRT semver: bug fixes can absolutely be major. If the same function call returns different results before and after given the same environment, then it’s major. There is a 100% chance that someone somewhere for some wicked reason is relying on the old broken behaviour. |
| expectedPorts: 1, | ||
| }, | ||
| } { | ||
| t.Run(fmt.Sprintf("List ports by %s", tt.name), func(t *testing.T) { |
There was a problem hiding this comment.
What does that mean?
| } | ||
| logPorts() | ||
| t.Fatalf("Returned ports did not contain expected port") | ||
| }() |
There was a problem hiding this comment.
oh I get it. you're saving us one "found" bool
There was a problem hiding this comment.
Yeah, the early return. I think the first version of this was returning something, which is normally why I write this. I can change it if it's majorly objectionable.
|
@pierreprinetti Did you need any changes before approval? |
Not on my side. |
pierreprinetti
left a comment
There was a problem hiding this comment.
Thank you for fixing this, LGTM.
Fixes #2807
Links to the line numbers/files in the OpenStack source code that support the
code in this PR:
Absolutely no idea! I can't even find the entry point for the ports api handler. This is reverse engineered from the behaviour from the cli client, which works.