Skip to content
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.

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

loadbalancer: Use CreateMemberOpts instead of BatchUpdateMemberOpts in PoolCreateOpts #2560

Merged
merged 3 commits into from
Aug 2, 2023

Conversation

danfai
Copy link
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

…n PoolCreateOpts

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

@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.

@EmilienM
Copy link
Contributor

EmilienM commented Mar 1, 2023

cc @dulek

@EmilienM
Copy link
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
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

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
Contributor

dulek commented Mar 6, 2023

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

@dulek
Copy link
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
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
Contributor

dulek commented Mar 7, 2023

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
2 checks passed
@pierreprinetti
Copy link
Contributor

@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
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
Labels
semver:major Breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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