Adopt metal-go

[codinja1188]

What this PR does / why we need it:

This PR replaces packngo with metal-go for all interactions with the Equinix Metal API. The metal-go SDK is generated from the Equinix Metal API spec, and we will gradually replace all usage of packngo with metal-go to streamline support.

Which issue(s) this PR fixes (optional, in fixes #(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

[displague]

I believe we will want to preserve ListOptions in some way.

In Metal Go, request types that include Include() and Exclude() could be used as the basis for an interface that a new ListOptions would depend on.

We could take advantage of the generated methods (https://github.com/equinix-labs/metal-go/blob/main/metal/v1/api_devices.go#L831-L841).

type RequestWithOptions interface {
  Include([]string)
  Exclude([]string)
}

And create a ListOptions function that returns a function that manipulates the Request object:

func (c *Client) ListOptions(defaultIncludes, defaultExcludes []string) func(RequestWithOptions ) {
  // essentially the existing code, building `[]string` for
  // `includes` and `excludes` rather than stashing it
  // into the ListOptions variable.
  return func(RequestWithOptions req) {
    res.Include(includes)
    res.Exclude(excludes)    
  }
}

The caller would then pass their request into the returned function to update the request object based on the options. This approach would be in an attempt to minimize the code change needed from action to action.

Where this seems to fall apart is that we don't have spec definitions for search, sort-by, and sort-dir. These could be added to the API spec (via patches) case-by-case for the APIs where metal-cli needs them. The returned function then becomes func(interface{}) and we would need type assertions to verify if the Request object implements RequestWithOptions, RequestWithSearch, RequestWithSort, etc. before manipulating the object with the options.

More complex is that In order to apply the filter (an arbitrary set of key/value pairs: map[string]string), we would need some function that could append additional query parameters, either well-known (defined in spec, with helper methods) or not (arbitrary query parameters).

Thoughts, @ctreatma?

[displague]

Alternatively, we rip out the ListOptions helper and do this by hand for each action until a better pattern emerges. 😕

[ctreatma]

The generated SDKs are extremely opinionated, which is what makes them so good at providing spec feedback. If we start trying to bolt packngo design patterns on to metal-go, I think we'd be better off killing metal-go and keeping packngo as the supported Go SDK.

In terms of the missing parameters in the spec, we should get those updated upstream; patches are nice but it can be a pain when they're upstreamed, and these parameters are a clear case of something that should be covered in the spec (and by extension, the redoc docs).

[displague]

@ctreatma were there examples in your other 'adopt metal-go' PRs that could serve as guidance for these query parameters?

• feat: use metal-go for device reads (data source and resource) terraform-provider-equinix#291
• 🏃 Adopt metal-go kubernetes-sigs/cluster-api-provider-packet#527

[ctreatma]

The terraform PR makes use of Include in some places, such as here: https://github.com/equinix/terraform-provider-equinix/pull/291/files#diff-dd9b87265483ecd6ba13ac77846bf80816e2efc1b654fb84e897155fda5e89dfR224

The request models are chainable, so using both include and exclude parameters could look something like obj, resp, err := req.Include(includes).Exclude(excludes).Execute(). Once the other parameters are in the spec, the generated code would include similar methods for requests that support those parameters as well, so you'd potentially end up with something like req.Include(includes).Exclude(excludes).Filter(filter).Search(search).SortBy(sortBy).Execute() (with better formatting, but there's only so much that formatting can do with that). If I had to guess, it would actually look like a bunch of if statements so we only call, e.g. req.Search() if there is a non-nil value for search.

[displague]

Sounds good. Filter is a packngo-ism. It represents arbitrary query string parameters. These happen to be useful for filtering ("metro=sv").

Does metal-go have a way to tack-on query parameters (ideally from a map[string]string parameter) in a request?

[displague]

it would actually look like a bunch of if statements so we only call

I believe the API won't complain (or change its response) if empty strings are provided for these parameters. I agree that it would be better to not chance that. I wonder if openapitools-generator's Go will include omitempty for these string fields based on some OAS3 spec. I see https://swagger.io/docs/specification/describing-parameters/#nullable, but that "allowEmptyValue" is about permitting ?foo&bar, not omitting those fields from the query string.

[ctreatma]

We may have an easier time adopting metal-go if we do it gradually. That would look similar to the metal-go PR for the terraform provider, where we set up both a packngo client and a metal-go client: equinix/terraform-provider-equinix#291

[ocobles]

AlwaysPxe is a pointer in metal-go but it's not in packngo. In packngo there was no distinction between unset and zero values, so false (default value in line 143) was an empty value for omitempty, but now it is false. This causes the creation of a device with OS other than ipxe to fail in API since it shouldn't be specified.

I would rather check wether is empty and only include it depending on the operatingSystem field. Behavior of ipxescripturl, hardwareReservationID, spotInstance, spotPriceMax may need to be reviewed as well

[ctreatma]

Yeah, this same issue came up in the cluster-api provider. This is the change we needed over there to only set the hardware reservation if one was specified: https://github.com/kubernetes-sigs/cluster-api-provider-packet/pull/527/files#diff-febb66857beae6d70b0aad1c73c92369afe1fb7d309f699f6047af7912d99e05R187-R189

[cprivitere]

Sounds like we've decided to instead break this up into smaller PRs like #282 #284 and #285 . Shall we close this @displague and @ctreatma