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: upgrade metal-go to 0.22.2 and fix enum type errors #350

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

ctreatma
Copy link
Contributor

The latest release of metal-go introduced true enum fields. Previously, metal-go generated enum fields as plain strings due to a bug in openapi-generator. Now that enum fields are generated as Go enums, we need to convert user-provided string values for those fields to the correct enum type for input and we need to convert the enum values to plain strings for output.

This updates metal-go to the latest release and addresses type errors for metal-go client code as it exists in the main branch; PRs that introduce metal-go elsewhere in the CLI may need to be updated as well before they can be merged.

@ctreatma ctreatma temporarily deployed to internal September 20, 2023 17:45 — with GitHub Actions Inactive
@@ -44,12 +44,11 @@ func (c *Client) Retrieve() *cobra.Command {

RunE: func(cmd *cobra.Command, args []string) error {
cmd.SilenceUsage = true
include := []string{"Inner_example"}
exclude := []string{"Inner_example"}
withoutProjects := "withoutProjects_example"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The withoutProjects option did not exist prior to metal-go conversion, and is not wired up to any user-facing flags, so I've opted to remove it rather than fix the type error for now.

@@ -108,13 +108,13 @@ func GetAllEvents(s metal.ApiFindEventsRequest) ([]metal.Event, error) {
}
}

func GetAllOrganizations(s metal.OrganizationsApiService, include, exclude []string, withOutProjects string) ([]metal.Organization, error) {
func GetAllOrganizations(s metal.OrganizationsApiService, include, exclude []string) ([]metal.Organization, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the withoutProjects setting from the organization get subcommand means we can also remove it here.

request = request.Type_(filters["type"])
validType, err := metal.NewFindPlansTypeParameterFromValue(filters["type"])
if err != nil {
return err
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted to just return the error here rather than wrapping it, since the error that's thrown is already helpful: https://github.com/equinix-labs/metal-go/blob/main/metal/v1/model_plan_type.go#L60

for _, role := range roles {
validRole, err := metal.NewInvitationRolesInnerFromValue(role)
if err != nil {
return nil, err
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again opted to return the original error without wrapping it, since the message looks good to me: https://github.com/equinix-labs/metal-go/blob/main/metal/v1/model_invitation_roles_inner.go#L62

if billingCycle != "" {
validBillingCycle, err = metal.NewDeviceCreateInputBillingCycleFromValue(billingCycle)
if err != nil {
return err
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is returning the original error because that message is nicely descriptive: https://github.com/equinix-labs/metal-go/blob/main/metal/v1/model_device_create_input_billing_cycle.go#L52

The error message does make mention of the type DeviceCreateInputBillingCycle; it would be preferable to reference either the CLI flag name or the attribute name there, but IMO tweaking that is not worth the effort at the moment.

@ctreatma
Copy link
Contributor Author

The API spec for the plan type enum has NVME, but the API response has NVMe, so the plan subcommand is broken by the introduction of true enums: https://github.com/equinix/metal-cli/actions/runs/6252218404/job/16974991464#step:5:116

@ctreatma ctreatma temporarily deployed to internal September 20, 2023 21:20 — with GitHub Actions Inactive
@ctreatma ctreatma changed the title chore: upgrade metal-go to 0.22 and fix enum type errors chore: upgrade metal-go to 0.22.1 and fix enum type errors Sep 20, 2023
@ctreatma ctreatma temporarily deployed to internal September 20, 2023 21:54 — with GitHub Actions Inactive
@ctreatma
Copy link
Contributor Author

There were other data validation issues that I missed somehow when running these tests locally; another metal-go PR is up to remove a couple inaccurate enums from the plan spec: equinix-labs/metal-go#156

Copy link
Contributor

@codinja1188 codinja1188 left a comment

Choose a reason for hiding this comment

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

/LGTM

@ctreatma ctreatma temporarily deployed to internal September 21, 2023 18:23 — with GitHub Actions Inactive
@ctreatma ctreatma changed the title chore: upgrade metal-go to 0.22.1 and fix enum type errors chore: upgrade metal-go to 0.22.2 and fix enum type errors Sep 21, 2023
@ctreatma
Copy link
Contributor Author

Updated to run with [email protected] and the end-to-end tests are passing again

@cprivitere cprivitere merged commit a264ffd into main Sep 21, 2023
9 checks passed
@cprivitere cprivitere deleted the metal-go-enums branch September 21, 2023 21:00
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.

3 participants