-
Notifications
You must be signed in to change notification settings - Fork 46
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: Updated metal-go client for sub-commands hardware reservations #288
Conversation
This has an extra commit in it and needs to be rebased. |
c24fc66
to
99b3d98
Compare
This has a conflict and needs a rebase. |
152fa80
to
f3f6923
Compare
want: &cobra.Command{}, | ||
cmdFunc: func(t *testing.T, c *cobra.Command) { | ||
root := c.Root() | ||
root.SetArgs([]string{subCommand, "get"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we need -i
or -p
and an ID here. Since we won't have any hardware reservations available to this E2E account, we should use a mocked client that will always return a stub reservation.
=== RUN TestCli_Hardware
=== RUN TestCli_Hardware/get
Error: either id or project-id should be set
Usage:
metal hardware-reservation get [-p <project_id>] | [-i <hardware_reservation_id>] [flags]
Aliases:
get, list
Examples:
# Retrieve all hardware reservations of a project:
metal hardware-reservations get -p $METAL_PROJECT_ID
# Retrieve the details of a specific hardware reservation:
metal hardware-reservations get -i 8404b73c-d18f-4190-8c49-20bb17501f88
Flags:
-h, --help help for get
-i, --id string The UUID of a hardware reservation.
-p, --project-id string A project's UUID.
Global Flags:
--config string Path to JSON or YAML configuration file
--exclude strings Comma separated Href references to collapse in results, may be dotted three levels deep
--filter stringArray Filter 'get' actions with name value pairs. Filter is not supported by all resources and is implemented as request query parameters.
--http-header strings Headers to add to requests (in format key=value)
--include strings Comma separated Href references to expand in results, may be dotted three levels deep
-o, --output string Output format (*table, json, yaml). env output formats are (*sh, terraform, capp).
--search string Search keyword for use in 'get' actions. Search is not supported by all resources.
--sort-by string Sort fields for use in 'get' actions. Sort is not supported by all resources.
--sort-dir string Sort field direction for use in 'get' actions. Sort is not supported by all resources.
--token string Metal API Token (METAL_AUTH_TOKEN)
hardware_test.go:45: either id or project-id should be set
hardware_test.go:56: expected output should include n3.xlarge.x86, m3.large.x86, s3.xlarge.x86, dc10.
--- FAIL: TestCli_Hardware (0.00s)
--- FAIL: TestCli_Hardware/get (0.00s)
FAIL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The terraform provider has an example of setting up a mock API with httptest
: https://github.com/equinix/terraform-provider-equinix/blob/main/equinix/resource_metal_device_acc_test.go#L1082-L1116
In this case, you would want to have your mock return a 200 with a JSON string that has the necessary fields. You could also consider the other mocking approaches documented here: https://medium.com/zus-health/mocking-outbound-http-requests-in-go-youre-probably-doing-it-wrong-60373a38d2aa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated mock client, and response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test mentioned earlier is still failing with the same error as before:
=== RUN TestCli_Hardware/get
Error: either id or project-id should be set
f3f6923
to
12ecbf2
Compare
internal/cli/root.go
Outdated
@@ -108,6 +108,11 @@ func (c *Client) metalApiConnect(httpClient *http.Client) error { | |||
configuration.Debug = checkEnvForDebug() | |||
configuration.AddDefaultHeader("X-Auth-Token", c.Token()) | |||
configuration.UserAgent = fmt.Sprintf("metal-cli/%s %s", c.Version, configuration.UserAgent) | |||
configuration.Servers = metal.ServerConfigurations{ | |||
metal.ServerConfiguration{ | |||
URL: c.apiURL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that this value is empty because the E2E tests are failing with an invalid API URL (it does not contain https://api.equinix.com/metal/v1
so requests are being made directly to /path/...
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed this in #432
|
internal/hardware/retrieve.go
Outdated
@@ -46,7 +27,8 @@ func (c *Client) Retrieve() *cobra.Command { | |||
|
|||
header := []string{"ID", "Facility", "Metro", "Plan", "Created"} | |||
|
|||
inc := []string{} | |||
inc := c.Servicer.Includes(nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved to after the switch
block a couple lines down so that we keep the user-specified includes in addition to the ones that are set automatically by the CLI.
internal/hardware/retrieve.go
Outdated
|
||
return c.Out.Output(r, header, &data) | ||
} | ||
request := c.Service.FindProjectHardwareReservations(context.Background(), projectID).Include(inc).Exclude(exc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This request isn't wired up to the CLI flags for include and exclude
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (the only test failure in CI is due to a platform failure while reinstalling a device)
Breakout from #270
What this PR does / why we need it:
This PR replaces packngo with metal-go for all interactions with the Equinix Metal API specifically for hardware sub commands
DISCUSSION POINTS:
Fixes #46