Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[neutron]: introduce Stateful argument for the security groups #3092

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

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

kayrus
Copy link
Contributor

@kayrus kayrus commented Jul 9, 2024

Prior to starting a PR, please make sure you have read our
contributor tutorial.

Prior to a PR being reviewed, there needs to be a Github issue that the PR
addresses. Replace the brackets and text below with that issue number.

Fixes #3091

Links to the line numbers/files in the OpenStack source code that support the
code in this PR:

https://github.com/openstack/neutron-lib/blob/47e1736c10ffd6f1850eba9970acdc00373f9de7/neutron_lib/api/definitions/stateful_security_group.py#L36-L42

@github-actions github-actions bot added the semver:major Breaking change label Jul 9, 2024
@coveralls
Copy link

Coverage Status

coverage: 78.742% (+0.005%) from 78.737%
when pulling 2b979bf on kayrus:secgroup-stateful
into 71268a5 on gophercloud:master.

@kayrus kayrus requested a review from a team July 9, 2024 19:31
@EmilienM
Copy link
Contributor

Since this is a breaking API change, I wonder if we shouldn't document it right away and explain the migration in MIGRATING.md or something cc @stephenfin @pierreprinetti

@kayrus
Copy link
Contributor Author

kayrus commented Jul 10, 2024

@EmilienM hm, this is not a breaking change. The change is absolutely backwards compatible.

@mandre
Copy link
Contributor

mandre commented Jul 10, 2024

@EmilienM hm, this is not a breaking change. The change is absolutely backwards compatible.

We changed the signature of List() to take a ListOptsBuilder rather than a ListOpts.

You can check the output of go-apidiff, with what seems like a small reporting issue:

github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/groups
  Incompatible changes:
  - ListOpts: changed from ListOpts to ListOpts
  Compatible changes:
  - CreateOpts.Stateful: added
  - ListOptsBuilder: added
  - SecGroup.Stateful: added
  - UpdateOpts.Stateful: added'

@kayrus
Copy link
Contributor Author

kayrus commented Jul 10, 2024

@mandre since ListOpts completely satisfies the new ListOptsBuilder, from my perspective this is backwards compatible. So new dependency will compile with the app, which was previously compiled with an old dependency.

@mandre
Copy link
Contributor

mandre commented Jul 10, 2024

@mandre since ListOpts completely satisfies the new ListOptsBuilder, from my perspective this is backwards compatible. So new dependency will compile with the app, which was previously compiled with an old dependency.

Right, so this could be a bug in go-apidiff.

@EmilienM EmilienM merged commit 2a1c755 into gophercloud:master Jul 10, 2024
16 checks passed
@kayrus kayrus added the backport-v1 This PR will be backported to v1 label Jul 10, 2024
@kayrus kayrus deleted the secgroup-stateful branch July 10, 2024 14:15
Copy link

Labels semver-major or semver-unknown can not trigger backports.
The PR has to be labeled semver-patch or semver-minor.

@pierreprinetti
Copy link
Member

Merging this PR means that we now need a v2 branch and the related backport machinery! 😅
@mandre @EmilienM @kayrus

@pierreprinetti
Copy link
Member

@kayrus

@mandre since ListOpts completely satisfies the new ListOptsBuilder, from my perspective this is backwards compatible. So new dependency will compile with the app, which was previously compiled with an old dependency.

You changed a function signature, which means that you're breaking a statement like this one:

var myFunc func(*gophercloud.ServiceClient, secgroup.ListOpts) = secgroup.List

@kayrus
Copy link
Contributor Author

kayrus commented Jul 10, 2024

@pierreprinetti haven't thought about this, but fair enough. However it's not clear in how many places you can see such patterns.

@kayrus
Copy link
Contributor Author

kayrus commented Jul 10, 2024

@pierreprinetti shall I revert this PR?

@pierreprinetti
Copy link
Member

pierreprinetti commented Jul 10, 2024

it's not clear in how many places you can see such patterns

You'd be surprised. cluster-api-provider-openstack for example wraps all Gophercloud calls into custom interfaces, so that it can mock its logic.

@pierreprinetti
Copy link
Member

@pierreprinetti shall I revert this PR?

I don't think so. But if you want to use stateful in v2 or v1, you'll have to make custom backport PRs without the ListOpts change. and before that, we need to set up the v2 branch and associated machinery

@kayrus
Copy link
Contributor Author

kayrus commented Jul 15, 2024

@pierreprinetti I'm going to create a v2 branch with a corresponding backport automation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v1 This PR will be backported to v1 semver:major Breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add stateful argument to security groups
5 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.