Skip to content

Neutron v2: BGP Speaker create / delete #2395

Merged
mandre merged 1 commit intogophercloud:masterfrom
shhgs:20220502
May 7, 2022
Merged

Neutron v2: BGP Speaker create / delete #2395
mandre merged 1 commit intogophercloud:masterfrom
shhgs:20220502

Conversation

@shhgs
Copy link
Copy Markdown
Contributor

@shhgs shhgs commented May 4, 2022

Neutron V2: BGP Dynamic Routing
For #2208

$ git diff --stat HEAD^                                                                    
 acceptance/openstack/networking/v2/extensions/bgp/speakers/bgpspeakers_test.go | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++                                                      
 acceptance/openstack/networking/v2/extensions/bgp/speakers/doc.go              |  2 ++
 openstack/networking/v2/extensions/bgp/speakers/doc.go                         | 30 ++++++++++++++++++++++++++++++
 openstack/networking/v2/extensions/bgp/speakers/requests.go                    | 40 ++++++++++++++++++++++++++++++++++++++++
 openstack/networking/v2/extensions/bgp/speakers/results.go                     | 12 ++++++++++++
 openstack/networking/v2/extensions/bgp/speakers/testing/fixture.go             | 28 ++++++++++++++++++++++++++++
 openstack/networking/v2/extensions/bgp/speakers/testing/requests_test.go       | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 openstack/networking/v2/extensions/bgp/speakers/urls.go                        | 10 ++++++++++
 8 files changed, 245 insertions(+)

@shhgs shhgs force-pushed the 20220502 branch 2 times, most recently from 3aff71e to 757835e Compare May 4, 2022 19:58
@shhgs
Copy link
Copy Markdown
Contributor Author

shhgs commented May 4, 2022 •
edited
Loading

I just noticed that there's a mistake in the 2 PRs that have already been merged.

They are for Neutron V2: BGP Dynamic Routing, but was mistakenly tagged for #2209. Is it possible to fix it?

Also I noticed this PR is shown in both #2208 and #2209. Is there anything wrong with this PR?

@coveralls
Copy link
Copy Markdown

coveralls commented May 4, 2022 •
edited
Loading

Coverage Status

Coverage increased (+0.006%) to 79.951% when pulling 0135343 on shhgs:20220502 into 14457d6 on gophercloud:master.

@mandre
Copy link
Copy Markdown
Contributor

mandre commented May 5, 2022

Thanks for the PR, I'll make sure to have a look at it tomorrow when I have more free time.

I just noticed that there's a mistake in the 2 PRs that have already been merged.

* [Neutron v2: BGP Peer list / get #2241](https://github.com/gophercloud/gophercloud/pull/2241)

* [Neutron v2: BGP Peer create / delete #2388](https://github.com/gophercloud/gophercloud/pull/2388)

They are for Neutron V2: BGP Dynamic Routing, but was mistakenly tagged for #2209. Is it possible to fix it?

Not really an issue as long as your correctly reference your two PRs in #2208.

Also I noticed this PR is shown in both #2208 and #2209. Is there anything wrong with this PR?

Github magic! That's because you referenced both issues from this PR that it shows on the issues page.

mandre
mandre reviewed May 6, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@mandre mandre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice. Could you just make sure you also call the List() method when you have speakers in your acceptance tests as we don't have coverage for it otherwise?

acceptance/openstack/networking/v2/extensions/bgp/speakers/bgpspeakers_test.go Outdated
allSpeakers, err := speakers.ExtractBGPSpeakers(allPages)
th.AssertNoErr(t, err)

t.Logf("Retrieved BGP Speakers")
Copy link
Copy Markdown
Contributor

@mandre mandre May 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you intentionally retrieved BGP speakers before creating one? It's perhaps also worth getting the list of BGP speakers once we have one.

Copy link
Copy Markdown
Contributor Author

@shhgs shhgs May 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I run this test on our lab, where it already have speakers. I will take your advice and modify the test.

Copy link
Copy Markdown
Contributor Author

@shhgs shhgs May 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mandre
I adjusted the test order. Can you please review again?

mandre
mandre approved these changes May 7, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@mandre mandre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, LGTM.

@mandre mandre merged commit 2cc0abf into gophercloud:master May 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mandre mandre mandre approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@shhgs @coveralls @mandre