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

chore: Migrate gateways command from packngo to metal-go client #376

Merged
merged 9 commits into from
Nov 22, 2023

Conversation

aayushrangwala
Copy link
Contributor

Issue Task as part of migrating metal-cli from packngo to metal-go client, added the support of gateways subcommand to use metal-go
Fixes: #333

Add tests for gateway subcommand

Signed-off-by: Ayush Rangwala <ayush.rangwala@gmail.com>
Signed-off-by: Ayush Rangwala <ayush.rangwala@gmail.com>
@aayushrangwala aayushrangwala temporarily deployed to external November 1, 2023 16:26 — with GitHub Actions Inactive
@aayushrangwala aayushrangwala changed the title fix: Migrate gateways command from packngo to metal-go client chore: Migrate gateways command from packngo to metal-go client Nov 7, 2023
@displague
Copy link
Member

=== RUN   TestGateways_Retrieve
Error when calling `DevicesApi.FindDeviceById`: 403 Forbidden
    helper.go:323: 403 Forbidden
    helper.go:326: Timeout while waiting for device: 501ed6e8-b24c-4bb8-b5db-bf187152c77a to be active
Error when calling `DevicesApi.FindDeviceById`: 403 Forbidden
    helper.go:331: 403 Forbidden
=== RUN   TestGateways_Retrieve/retrieve_gateways_by_projectId
--- FAIL: TestGateways_Retrieve (220.20s)
    --- FAIL: TestGateways_Retrieve/retrieve_gateways_by_projectId (0.22s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0xa0 pc=0xb7587b]

goroutine 45 [running]:
testing.tRunner.func1.2({0xc34f00, 0x12d9410})
	/opt/hostedtoolcache/go/1.19.13/x64/src/testing/testing.go:13[96](https://github.com/equinix/metal-cli/actions/runs/6786304964/job/18533442407?pr=376#step:5:97) +0x24e
testing.tRunner.func1()
	/opt/hostedtoolcache/go/1.19.13/x64/src/testing/testing.go:13[99](https://github.com/equinix/metal-cli/actions/runs/6786304964/job/18533442407?pr=376#step:5:100) +0x39f
panic({0xc34f00, 0x12d9410})
	/opt/hostedtoolcache/go/1.19.13/x64/src/runtime/panic.go:884 +0x212
github.com/equinix/metal-cli/test/e2e/gateways.TestGateways_Retrieve.func4(0xc000453380, 0xc000206f50?)
	/home/runner/work/metal-cli/metal-cli/test/e2e/gateways/retrieve_test.go:83 +0x2bb
github.com/equinix/metal-cli/test/e2e/gateways.TestGateways_Retrieve.func5(0x1?)
	/home/runner/work/metal-cli/metal-cli/test/e2e/gateways/retrieve_test.go:92 +0x74
testing.tRunner(0xc000453380, 0xc0002bfae8)
	/opt/hostedtoolcache/go/1.19.13/x64/src/testing/testing.go:1446 +0x10b
created by testing.(*T).Run
	/opt/hostedtoolcache/go/1.19.13/x64/src/testing/testing.go:1493 +0x35f
FAIL	github.com/equinix/metal-cli/test/e2e/gateways	587.683s

@displague
Copy link
Member

=== RUN   TestPorts_VLANs
Error when calling `DevicesApi.FindDeviceById`: 403 Forbidden You are not authorized to view this device
    helper.go:323: 403 Forbidden You are not authorized to view this device
    helper.go:326: Timeout while waiting for device: 6a494e07-2687-4ff2-88e7-892e9d7583b2 to be active
Error when calling `DevicesApi.FindDeviceById`: 403 Forbidden You are not authorized to view this device
    helper.go:331: 403 Forbidden You are not authorized to view this device
--- FAIL: TestPorts_VLANs (107.45s)
panic: runtime error: index out of range [2] with length 0 [recovered]
	panic: runtime error: index out of range [2] with length 0

goroutine 33 [running]:
testing.tRunner.func1.2({0xcac120, 0xc000362420})
	/opt/hostedtoolcache/go/1.19.13/x64/src/testing/testing.go:1396 +0x24e
testing.tRunner.func1()
	/opt/hostedtoolcache/go/1.19.13/x64/src/testing/testing.go:1399 +0x39f
panic({0xcac120, 0xc000362420})
	/opt/hostedtoolcache/go/1.19.13/x64/src/runtime/panic.go:884 +0x212
github.com/equinix/metal-cli/test/e2e/ports.TestPorts_VLANs(0xc0002c16c0)
	/home/runner/work/metal-cli/metal-cli/test/e2e/ports/vlans_test.go:28 +0x4dc
testing.tRunner(0xc0002c16c0, 0xd6d890)
	/opt/hostedtoolcache/go/1.19.13/x64/src/testing/testing.go:1446 +0x10b
created by testing.(*T).Run
	/opt/hostedtoolcache/go/1.19.13/x64/src/testing/testing.go:1493 +0x35f
FAIL	github.com/equinix/metal-cli/test/e2e/ports	535.031s

@displague
Copy link
Member

I think SetupDeviceAndProject may need to t.Fatalf instead of t.Errorf when a device and project can't be created or loaded. We are finding cases where the tests failed early on during setup and we are only detecting that when device properties are being accessed (device.Metro.GetCode()).

@displague
Copy link
Member

displague commented Nov 13, 2023

We should also be calling t.Helper() in

func SetupProjectAndDevice(t *testing.T, projectId, deviceId *string) *openapiclient.Device {

@aayushrangwala
Copy link
Contributor Author

I think SetupDeviceAndProject may need to t.Fatalf instead of t.Errorf when a device and project can't be created or loaded. We are finding cases where the tests failed early on during setup and we are only detecting that when device properties are being accessed (device.Metro.GetCode()).

it was kept with t.Errorf(err) because I wanted to make the defer funcs run at every test function return. For t.Fatal() cleanups wont happen via defer and dangling resources will remain.
But fixed the calls to return an error from setup func when there is an error

Signed-off-by: Ayush Rangwala <ayush.rangwala@gmail.com>
@aayushrangwala
Copy link
Contributor Author

@displague @ctreatma Please trigger E2E tests

rootClient := root.NewClient(consumerToken, apiURL, Version)

device, err := helper.SetupProjectAndDevice(t, &projectId, &deviceId)
defer func() {
Copy link
Contributor Author

@aayushrangwala aayushrangwala Nov 16, 2023

Choose a reason for hiding this comment

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

use t.Cleanup() refer here

@@ -72,6 +72,27 @@ func CreateTestVLAN(projectId string) (*openapiclient.VirtualNetwork, error) {
return vlan, nil
}

func CreateTestGateway(projectId, vlanId string, privateIPv4SubnetSize *int32) (*openapiclient.MetalGateway, error) {
TestApiClient := TestClient()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the Helpers in this file needs to call t.Helper

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Handle that under a separate Issue and PR

Signed-off-by: Ayush Rangwala <ayush.rangwala@gmail.com>
Signed-off-by: Ayush Rangwala <ayush.rangwala@gmail.com>
@aayushrangwala
Copy link
Contributor Author

aayushrangwala commented Nov 21, 2023

Ran the tests locally for gateway and its running fine

❯ go test ./test/e2e/gateways
ok  	github.com/equinix/metal-cli/test/e2e/gateways	377.900s

Signed-off-by: Ayush Rangwala <ayush.rangwala@gmail.com>
@aayushrangwala
Copy link
Contributor Author

@ctreatma E2E tests failed but with a device test related error

=== RUN   TestCli_Devices_Update/reinstall_device
Error when calling `DevicesApi.FindDeviceById`: 403 Forbidden You are not authorized to view this device
Error when calling `DevicesApi.FindDeviceById`: 403 Forbidden You are not authorized to view this device
    device_reinstall_test.go:53: 403 Forbidden You are not authorized to view this device
--- FAIL: TestCli_Devices_Update (88.43s)
    --- FAIL: TestCli_Devices_Update/reinstall_device (88.43s)

All the ports, gateway tests are passing
Is it good to merge the PR

@ctreatma
Copy link
Contributor

I re-ran the tests a few times to try to get a passing run again, but couldn't. That's a bit concerning, but outside the scope of this PR, so this is fine to merge.

@ctreatma ctreatma merged commit 1994e1e into equinix:main Nov 22, 2023
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adopt metal-go
3 participants