Migrates cloudup/gce from v0.beta => v1 #8517
Conversation
geojaz
added
the
do-not-merge/work-in-progress
k8s-ci-robot
added
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: geojaz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/retest |
| return nil | ||
| }) | ||
| req := c.Compute().InstanceGroupManagers.ListManagedInstances(project, zoneName, igm.Name) | ||
| resp, err := req.Do() |
There was a problem hiding this comment.
As someone unfamiliar with the GCE api, I see that theres a PageToken for ListManagedInstances but I'm having trouble finding example code that uses it so I can't tell if we should continue to use pagination or not. Do you think that pagination is no longer necessary?
There was a problem hiding this comment.
It is unlikely in most cases that it would be needed, but you're right fixing...
There was a problem hiding this comment.
Well this is odd - it looks like the Pages method isn't defined there? I can try to dig around to find out why but (1) do we agree there is no Pages method and (2) did we figure out a reason why it might not be needed?
There was a problem hiding this comment.
@geojaz Pages was added in v0.22.0 (the compute-gen.go file is too big for me to link to a specific line or commit) so we should consider updating the api client library once more and then revert this particular change.
| if i.TargetSize == 0 { | ||
| request.ForceSendFields = append(request.ForceSendFields, "TargetSize") | ||
| } | ||
| op, err := t.Cloud.Compute().InstanceGroupManagers.ResizeAdvanced(t.Cloud.Project(), *e.Zone, i.Name, request).Do() |
There was a problem hiding this comment.
Interesting! The ForceSendFields was originally here because otherwise we didn't send Size == 0, but it looks like this has been fixed by using the Resize command.
| return nil | ||
| }) | ||
| req := c.Compute().InstanceGroupManagers.ListManagedInstances(project, zoneName, igm.Name) | ||
| resp, err := req.Do() |
There was a problem hiding this comment.
Well this is odd - it looks like the Pages method isn't defined there? I can try to dig around to find out why but (1) do we agree there is no Pages method and (2) did we figure out a reason why it might not be needed?
|
Ah - I'm plainly following in your footsteps @geojaz ... looks like googleapis/google-api-go-client#451 is where you asked in the library about the missing method, and so we're likely (temporarily) blocked on this question. I say temporarily because we could always code our own version of the method. |
|
that's exactly how I got there :) I've been busy and this isn't blocking me right now.... |
|
/close Now that we have the support in the client lib for that MIG listing, I'll recreate... |
k8s-ci-robot
added
the
needs-rebase
|
@geojaz: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@geojaz: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This relates to #8516 and is a first pass at modernizing our GCE integration.
#8516 needs to go first to get this green...We got the vendoring updated so when the tests are green, we can go on this one