feat: Add support for bgp dynamic neighbours subcommand for metal gateway by aayushrangwala · Pull Request #433 · equinix/metal-cli

aayushrangwala · 2024-01-15T17:42:16Z

Fixes: #413

ctreatma

Can you add tests for this subcommand?

aayushrangwala · 2024-01-26T16:56:15Z

Can you add tests for this subcommand?

@ctreatma I am working on the tests but should we keep small PRs and raise tests in the next one? It'll be easier to review as well. And anyway this is a new functionality which is not release

ctreatma · 2024-01-26T17:26:38Z

@ctreatma I am working on the tests but should we keep small PRs and raise tests in the next one? It'll be easier to review as well. And anyway this is a new functionality which is not release

Tests build the reviewer's confidence that the changes actually work the way they are supposed to. Doing them in a separate PR only serves to delay finishing the code.

ctreatma · 2024-01-26T19:26:11Z

+	"github.com/equinix/metal-cli/test/helper"
+)
+
+func TestBgpDynamicNeighbours_Create(t *testing.T) {


All of the new tests are failing with 404s:

=== RUN TestBgpDynamicNeighbours_Create/create_bgp_dynamic_neighbour Error: Could not create BGP Dynamic Neighbour: 404 Not Found Not found helper.go:591: Could not create BGP Dynamic Neighbour: 404 Not Found Not found --- FAIL: TestBgpDynamicNeighbours_Create (2.54s) --- FAIL: TestBgpDynamicNeighbours_Create/create_bgp_dynamic_neighbour (0.14s) === RUN TestBgpDynamicNeighbours_Delete delete_test.go:27: Error when calling `VRFsApi.CreateBgpDynamicNeighbor`: 404 Not Found Not found --- FAIL: TestBgpDynamicNeighbours_Delete (3.10s) === RUN TestBgpDynamicNeighbours_Get get_test.go:25: Error when calling `VRFsApi.CreateBgpDynamicNeighbor`: 404 Not Found Not found --- FAIL: TestBgpDynamicNeighbours_Get (2.65s) === RUN TestBgpDynamicNeighbours_List list_test.go:25: Error when calling `VRFsApi.CreateBgpDynamicNeighbor`: 404 Not Found Not found --- FAIL: TestBgpDynamicNeighbours_List (3.00s) FAIL

Do we need to pass in any additional CLI flags?

Not sure why its failing with 404 on the CI, its passing at local

~/go/src/metal-cli   gw-bgp-neighbours *2 !15 ?4 ❯ go test -v ./test/e2e/gateways/bgp-dynamic-neighbour/...  27s === RUN TestBgpDynamicNeighbors_Create === RUN TestBgpDynamicNeighbors_Create/create_bgp_dynamic_neighbor --- PASS: TestBgpDynamicNeighbors_Create (6.15s) --- PASS: TestBgpDynamicNeighbors_Create/create_bgp_dynamic_neighbor (0.83s) === RUN TestBgpDynamicNeighbors_Delete === RUN TestBgpDynamicNeighbors_Delete/delete_bgpDynamicNeighbor_by_ID --- PASS: TestBgpDynamicNeighbors_Delete (6.11s) --- PASS: TestBgpDynamicNeighbors_Delete/delete_bgpDynamicNeighbor_by_ID (0.82s) === RUN TestBgpDynamicNeighbors_Get === RUN TestBgpDynamicNeighbors_Get/get_bgpDynamicNeighbor_by_ID --- PASS: TestBgpDynamicNeighbors_Get (5.97s) --- PASS: TestBgpDynamicNeighbors_Get/get_bgpDynamicNeighbor_by_ID (0.43s) === RUN TestBgpDynamicNeighbors_List === RUN TestBgpDynamicNeighbors_List/list_bgpDynamicNeighbor_by_ID --- PASS: TestBgpDynamicNeighbors_List (6.04s) --- PASS: TestBgpDynamicNeighbors_List/list_bgpDynamicNeighbor_by_ID (0.66s) PASS ok github.com/equinix/metal-cli/test/e2e/gateways/bgp-dynamic-neighbour 24.716s

I'm also getting the 404 Not found Not Found on my local tests.

displague · 2024-01-29T18:22:59Z

I enabled VRF over Shared Connections, VRF Static Routers, and BGP Dynamic Neighbors for the testing org. We should see better progress in the E2E.

aayushrangwala · 2024-01-30T04:40:41Z

strings.Contains(err.Error(), "Not Found")

Yes after enabling FFs, tests started passing for BGP
https://github.com/equinix/metal-cli/actions/runs/7700595014/job/20986048893?pr=433#step:5:111

…eway Signed-off-by: Ayush Rangwala <[email protected]>

Signed-off-by: Ayush Rangwala <[email protected]>

Signed-off-by: Chris Privitere <[email protected]>

aayushrangwala · 2024-01-31T04:34:49Z

=== RUN   TestBgpDynamicNeighbors_Create
=== RUN   TestBgpDynamicNeighbors_Create/create_bgp_dynamic_neighbor
--- PASS: TestBgpDynamicNeighbors_Create (5.21s)
    --- PASS: TestBgpDynamicNeighbors_Create/create_bgp_dynamic_neighbor (0.45s)
=== RUN   TestBgpDynamicNeighbors_Delete
=== RUN   TestBgpDynamicNeighbors_Delete/delete_bgpDynamicNeighbor_by_ID
--- PASS: TestBgpDynamicNeighbors_Delete (3.82s)
    --- PASS: TestBgpDynamicNeighbors_Delete/delete_bgpDynamicNeighbor_by_ID (0.36s)
=== RUN   TestBgpDynamicNeighbors_Get
=== RUN   TestBgpDynamicNeighbors_Get/get_bgpDynamicNeighbor_by_ID
--- PASS: TestBgpDynamicNeighbors_Get (3.22s)
    --- PASS: TestBgpDynamicNeighbors_Get/get_bgpDynamicNeighbor_by_ID (0.19s)
=== RUN   TestBgpDynamicNeighbors_List
=== RUN   TestBgpDynamicNeighbors_List/list_bgpDynamicNeighbor_by_ID
--- PASS: TestBgpDynamicNeighbors_List (3.61s)
    --- PASS: TestBgpDynamicNeighbors_List/list_bgpDynamicNeighbor_by_ID (0.19s)

All related tests are passing in this test run @ctreatma can you please approve this PR

Signed-off-by: Ayush Rangwala <[email protected]>

ctreatma

The only test failure in CI was a panic due to hitting the go test timeout. I'm going to assume it's unrelated and deal with any consequences later.

aayushrangwala had a problem deploying to external January 15, 2024 17:42 — with GitHub Actions Error

aayushrangwala force-pushed the gw-bgp-neighbours branch from b877e93 to 9eb41dd Compare January 15, 2024 17:45

aayushrangwala temporarily deployed to external January 15, 2024 17:46 — with GitHub Actions Inactive

displague reviewed Jan 16, 2024

View reviewed changes

Comment thread internal/gateway/bgp-dynamic-neighbours/create.go Outdated

displague marked this pull request as draft January 16, 2024 21:44

aayushrangwala force-pushed the gw-bgp-neighbours branch from 9eb41dd to 17aa269 Compare January 23, 2024 15:53

aayushrangwala had a problem deploying to external January 23, 2024 15:53 — with GitHub Actions Error

aayushrangwala force-pushed the gw-bgp-neighbours branch from 17aa269 to 4d1bc15 Compare January 24, 2024 19:16

aayushrangwala had a problem deploying to external January 24, 2024 19:16 — with GitHub Actions Error

aayushrangwala force-pushed the gw-bgp-neighbours branch from 4d1bc15 to 1c1bdb5 Compare January 25, 2024 13:05

aayushrangwala had a problem deploying to external January 25, 2024 13:05 — with GitHub Actions Error

aayushrangwala temporarily deployed to external January 25, 2024 13:06 — with GitHub Actions Inactive

aayushrangwala marked this pull request as ready for review January 25, 2024 13:06

ctreatma suggested changes Jan 25, 2024

View reviewed changes

aayushrangwala temporarily deployed to external January 26, 2024 18:35 — with GitHub Actions Inactive

ctreatma reviewed Jan 26, 2024

View reviewed changes

aayushrangwala temporarily deployed to external January 28, 2024 15:28 — with GitHub Actions Inactive

aayushrangwala temporarily deployed to external January 29, 2024 16:15 — with GitHub Actions Inactive

aayushrangwala force-pushed the gw-bgp-neighbours branch from 0fd6301 to 268a1ff Compare January 29, 2024 16:51

aayushrangwala temporarily deployed to external January 29, 2024 16:51 — with GitHub Actions Inactive

aayushrangwala force-pushed the gw-bgp-neighbours branch from 268a1ff to 0facfe8 Compare January 29, 2024 18:15

aayushrangwala temporarily deployed to external January 29, 2024 18:15 — with GitHub Actions Inactive

aayushrangwala requested a deployment to external January 29, 2024 18:50 — with GitHub Actions Abandoned

ctreatma reviewed Jan 29, 2024

View reviewed changes

Comment thread test/helper/helper.go Outdated

aayushrangwala temporarily deployed to external January 30, 2024 04:41 — with GitHub Actions Inactive

aayushrangwala requested a deployment to external January 30, 2024 16:07 — with GitHub Actions Abandoned

aayushrangwala temporarily deployed to external January 30, 2024 17:08 — with GitHub Actions Inactive

aayushrangwala force-pushed the gw-bgp-neighbours branch from 9e1f7a6 to e5c2c1f Compare January 30, 2024 17:39

aayushrangwala temporarily deployed to external January 30, 2024 17:40 — with GitHub Actions Inactive

aayushrangwala temporarily deployed to external January 30, 2024 20:01 — with GitHub Actions Inactive

aayushrangwala added 9 commits January 31, 2024 01:36

feat: Add support for bgp dynamic neighbours subcommand for metal gat…

7b7a724

…eway Signed-off-by: Ayush Rangwala <[email protected]>

Refactor bgp dynamic neighbour subcommand

7b7fb31

Signed-off-by: Ayush Rangwala <[email protected]>

generate new docs

c459e13

Signed-off-by: Ayush Rangwala <[email protected]>

Add tests for metal cli bgp-dynamic-neighbour

9a7072b

Signed-off-by: Ayush Rangwala <[email protected]>

Fix review comments

94c03cd

Print error and output on test metal cli command error

63e578c

Signed-off-by: Ayush Rangwala <[email protected]>

use resp.StatusCode in test helper

280a232

Signed-off-by: Ayush Rangwala <[email protected]>

add cleanup gateway

cdd963f

add wait for gateway to delete

6c4f34d

Signed-off-by: Ayush Rangwala <[email protected]>

aayushrangwala force-pushed the gw-bgp-neighbours branch from 32d0f63 to 6c4f34d Compare January 30, 2024 20:06

aayushrangwala temporarily deployed to external January 30, 2024 20:06 — with GitHub Actions Inactive

aayushrangwala requested a deployment to external January 30, 2024 21:44 — with GitHub Actions Abandoned

cprivitere reviewed Jan 30, 2024

View reviewed changes

Comment thread test/helper/helper.go

fix: two typos in the neighbor tests

638a97c

Signed-off-by: Chris Privitere <[email protected]>

cprivitere had a problem deploying to external January 30, 2024 23:15 — with GitHub Actions Error

fix: US english spelling instead of British

22cc0a0

Signed-off-by: Chris Privitere <[email protected]>

cprivitere had a problem deploying to external January 30, 2024 23:16 — with GitHub Actions Error

fix: managed to leave a typo in the fixed typo

1a267f7

Signed-off-by: Chris Privitere <[email protected]>

cprivitere had a problem deploying to external January 30, 2024 23:16 — with GitHub Actions Error

fix: right the other typo I missed

1b2bab5

Signed-off-by: Chris Privitere <[email protected]>

cprivitere temporarily deployed to external January 30, 2024 23:17 — with GitHub Actions Inactive

pass vlan metro in test

8fcadd3

Signed-off-by: Ayush Rangwala <[email protected]>

aayushrangwala temporarily deployed to external January 31, 2024 18:40 — with GitHub Actions Inactive

ctreatma approved these changes Jan 31, 2024

View reviewed changes

ctreatma merged commit 60d0035 into equinix:main Jan 31, 2024

Conversation

aayushrangwala commented Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ctreatma left a comment

Choose a reason for hiding this comment

Uh oh!

aayushrangwala commented Jan 26, 2024

Uh oh!

ctreatma commented Jan 26, 2024

Uh oh!

Uh oh!

ctreatma Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

aayushrangwala Jan 28, 2024

Choose a reason for hiding this comment

Uh oh!

cprivitere Jan 30, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

displague commented Jan 29, 2024

Uh oh!

Uh oh!

aayushrangwala commented Jan 30, 2024

Uh oh!

Uh oh!

aayushrangwala commented Jan 31, 2024

Uh oh!

ctreatma left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aayushrangwala commented Jan 15, 2024 •

edited

Loading