Skip to content

loadbalancer: Use CreateMemberOpts instead of BatchUpdateMemberOpts in PoolCreateOpts#2560

Merged
mandre merged 3 commits intogophercloud:masterfrom
danfai:fix-creating-fully-populated-lb
Aug 2, 2023
Merged

loadbalancer: Use CreateMemberOpts instead of BatchUpdateMemberOpts in PoolCreateOpts#2560
mandre merged 3 commits intogophercloud:masterfrom
danfai:fix-creating-fully-populated-lb

Conversation

@danfai
Copy link
Copy Markdown
Contributor

@danfai danfai commented Feb 16, 2023

In order to support members without any subnet, the struct for the pool members need to be CreateMemberOpts.

Fixes #2559

Links to the line numbers/files in the OpenStack source code that support the
code in this PR:
API validation for UUID in octavia:
https://opendev.org/openstack/octavia/src/commit/c2c59f4c9eb9f9ef6081e386bf2bc1badc145241/octavia/api/v2/types/member.py#L127

@danfai
loadbalancer: Use CreateMemberOpts instead of BatchUpdateMemberOpts i…
81fb82d 
…n PoolCreateOpts

In order to support members without any subnet, the struct for the pool
members need to be CreateMemberOpts.
github-actions[bot]
github-actions bot reviewed Feb 16, 2023
View reviewed changes
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.

@danfai danfai mentioned this pull request Feb 21, 2023
Closed
@EmilienM
Copy link
Copy Markdown
Contributor

EmilienM commented Mar 1, 2023

cc @dulek

@EmilienM
Copy link
Copy Markdown
Contributor

EmilienM commented Mar 1, 2023

Could you please update the tests accordingly, so at least we get happy CI jobs. Thanks

@EmilienM
Copy link
Copy Markdown
Contributor

EmilienM commented Mar 1, 2023

also FYI I marked this PR as non backward compatible since we're changing the type of an existing parameter in a struct.

@coveralls
Copy link
Copy Markdown

coveralls commented Mar 2, 2023

Coverage Status

Coverage: 80.237% (+0.1%) from 80.131% when pulling 207de4a on danfai:fix-creating-fully-populated-lb into c4fd26e on gophercloud:master.

@dulek
Copy link
Copy Markdown
Contributor

dulek commented Mar 6, 2023

I'm not sure about the CI, but the change makes sense to me.

@dulek
Copy link
Copy Markdown
Contributor

dulek commented Mar 6, 2023

Wait, are we allowed to break API like this? With this change SubnetID becomes string instead of *string. Same goes with Name and MonitorAddress.

@kayrus
Copy link
Copy Markdown
Contributor

kayrus commented Mar 6, 2023

Wait, are we allowed to break API like this?

previously this happened a lot, see #1332

@dulek
Copy link
Copy Markdown
Contributor

dulek commented Mar 7, 2023 •
edited
Loading

Alright, cool, then works for me.

@EmilienM EmilienM added the semver:major Breaking change label Jul 19, 2023
@mandre mandre merged commit 38ee8b7 into gophercloud:master Aug 2, 2023
@pierreprinetti
Copy link
Copy Markdown
Member

pierreprinetti commented Aug 7, 2023

@dulek @kayrus
Since we tagged Gophercloud v1.0.0, we do not allow breaking changes in the v1 branch. @mandre merged this PR to the development branch (master) with the semver:major label, which means that we won't backport to v1. We will only release this patch in a v2 branch and tag it v2.y.z.

@mandre
In the future, please make sure that PRs are well-squashed before merging them.

@mandre
Copy link
Copy Markdown
Contributor

mandre commented Aug 7, 2023

@mandre In the future, please make sure that PRs are well-squashed before merging them.

My bad. For my defense, I'll blame Github's terrible UI.

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

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@mandre mandre mandre approved these changes

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.

Loadbalancer create fully populated LB broken for empty subnet id in members

7 participants

@danfai @EmilienM @coveralls @dulek @kayrus @pierreprinetti @mandre