googleapis / google-api-go-client

Auto-generated Google APIs for Go.
https://pkg.go.dev/google.golang.org/api
BSD 3-Clause "New" or "Revised" License
3.98k stars 1.12k forks source link

InstanceGroupManager.Versions.TargetSize.Fixed and Percent Need To Be Pointers #2723

Closed ynikitin-etsy closed 1 day ago

ynikitin-etsy commented 1 month ago

Environment details

Steps to reproduce

package main

import (
    "context"
    "encoding/json"
    "fmt"

    "google.golang.org/api/compute/v1"
)

func main() {

    project := "myproject"
    region := "myregion"
    name := "mygroup"

    ctx := context.Background()
    computeService, err := compute.NewService(ctx)
    if err != nil {
        fmt.Printf("could not get service api: %v", err)
        return
    }

    manager, err := computeService.RegionInstanceGroupManagers.Get(project, region, name).Do()
    if err != nil {
        fmt.Printf("could not get manager call: %v", err)
        return
    }

    res2B, _ := json.MarshalIndent(manager.Versions, "", "   ")
    fmt.Println(string(res2B))

}

InstanceGroupManager.Versions.TargetSize with type *FixedOrPercent contains two important fields: Fixed int64, and Percent int64. When getting a response, no matter if only TargetSize.Fixed is set to 0 or only TargetSize.Percent is set to 0, both are returned as zero, since they are int64 and need default values. Thus, (importantly in context of gcp terraform provider), it is impossible to tell what value is actually set (fixed or percent).

type FixedOrPercent struct {
    Calculated int64 `json:"calculated,omitempty"`
    Fixed int64 `json:"fixed,omitempty"`
    Percent int64 `json:"percent,omitempty"`
    ForceSendFields []string `json:"-"`
    NullFields []string `json:"-"`
}

Those types should be Fixed *int64, and Percent *int64.

Sample raw API snippet response:

"versions": [
        {
            "name": "0/2024-04-16 13:20:28.567118+00:00",
            "instanceTemplate": "very-long-url-here-1",
            "targetSize": {
                "calculated": 4
            }
        },
        {
            "name": "1/2024-04-16 13:20:28.567118+00:00",
            "instanceTemplate": "very-long-url-here-2",
            "targetSize": {
                "fixed": 0,
                "calculated": 0
            }
        }
    ]

This is what the library returns as a response formatted into json.

[
   {
      "instanceTemplate": "very-long-url-here-1",
      "name": "0/2024-04-16 13:20:28.567118+00:00",
      "targetSize": {
         "calculated": 4
      }
   },
   {
      "instanceTemplate": "very-long-url-here-2",
      "targetSize": {}
   }
]

image

Zeros are valid values here. This change is needed to fix this bug in TF: https://github.com/hashicorp/terraform-provider-google/issues/14826

Thank you for any help or support!

codyoss commented 1 month ago

This is a general problem, especially in the compute surface for in Go with how default types are handled. I believe this is documented in TF. There are likely many more fields that could also be converted to pointers to help with this. I think in this case you can tease out what the value is as the docs for Fixed state: "Specifies a fixed number of VM instances. This must be a positive integer". The long term solution here would be for TF to use the newer compute client that makes most(all?) the surface pointer types: cloud.google.com/go/compute/apiv1. In the subpackage computepbthe equivalent type looks like:

// Encapsulates numeric value that can be either absolute or relative.
type FixedOrPercent struct {
    state         protoimpl.MessageState
    sizeCache     protoimpl.SizeCache
    unknownFields protoimpl.UnknownFields

    // [Output Only] Absolute value of VM instances calculated based on the specific mode. - If the value is fixed, then the calculated value is equal to the fixed value. - If the value is a percent, then the calculated value is percent/100 * targetSize. For example, the calculated value of a 80% of a managed instance group with 150 instances would be (80/100 * 150) = 120 VM instances. If there is a remainder, the number is rounded.
    Calculated *int32 `protobuf:"varint,472082878,opt,name=calculated,proto3,oneof" json:"calculated,omitempty"`
    // Specifies a fixed number of VM instances. This must be a positive integer.
    Fixed *int32 `protobuf:"varint,97445748,opt,name=fixed,proto3,oneof" json:"fixed,omitempty"`
    // Specifies a percentage of instances between 0 to 100%, inclusive. For example, specify 80 for 80%.
    Percent *int32 `protobuf:"varint,394814533,opt,name=percent,proto3,oneof" json:"percent,omitempty"`
}
ynikitin-etsy commented 3 weeks ago

Thank you for the response! I still don't see a clear way of figuring out the actual value, but I'll try a few things.

I obviously have no impact if the Terraform provider can use this different library, but will hope something eventually changes. Thank again!

codyoss commented 1 day ago

Closing this as working as intended for now. As mentioned above the best solution is to use the newer library that does not have these issues.