Add support for composite types for zonal resources #804
Conversation
k8s-ci-robot
added
cncf-cla: yes
prameshj
force-pushed
the
composite-zonal
branch
from
5554093 to
a22326a
Compare
|
/assign @bowei |
| } | ||
|
|
||
| {{if .HasUpdate}} | ||
| func Update{{.Name}}(gceCloud *gce.Cloud, key *meta.Key, {{.VarName}} *{{.Name}}) error { |
There was a problem hiding this comment.
this feels like a lot of cut and paste.
can we make this more succinct by parametrizing this on <scope, version> (e.g. <global, alpha>) etc so we only need to maintain one block of template text
There was a problem hiding this comment.
Are you referring to the duplicate code in lines 210-239 and 247 to 275? Those are the same except the operation is Create vs Update. We are following the same pattern for RegionalDefault and GlobalDefault resources.
prameshj
force-pushed
the
composite-zonal
branch
4 times, most recently
from
64d1412 to
05c8073
Compare
|
@spencerhance @bowei I have modified the template text to a single block for regional/global/zonal cases. PTAL, thanks. |
pkg/composite/gen.go
Outdated
| // ToForwardingRuleList converts a list of compute alpha, beta or GA | ||
| // ForwardingRule into a list of our composite type. | ||
| func ToForwardingRuleList(objs interface{}) ([]*ForwardingRule, error) { | ||
| result := []*ForwardingRule{} | ||
|
|
||
| err := copyViaJSON(&result, objs) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("Could not copy object %v to list of ForwardingRule via JSON: %v", objs, err) | ||
| return nil, fmt.Errorf("could not copy object %v to list of ForwardingRule via JSON: %v", objs, err) |
There was a problem hiding this comment.
can do
return nil, fmt.Errorf("could not copy object %v to %T via JSON: %v", objs, results, err)
There was a problem hiding this comment.
We use the template variable substitution {{type.Name}} to generate this. Fixed it to use %T.
pkg/composite/gen.go
Outdated
| switch key.Type() { | ||
| case meta.Zonal: | ||
| default: | ||
| return fmt.Errorf("Key %v not valid for zonal resource NetworkEndpointGroup %v", key.Type(), key.Name) |
There was a problem hiding this comment.
print the whole key instead of key.Type()
pkg/composite/gen.go
Outdated
| return nil, err | ||
| } | ||
| compositeType.Scope = meta.Zonal | ||
|
|
pkg/composite/gen.go
Outdated
| alpha := &computealpha.NetworkEndpointGroup{} | ||
| err := copyViaJSON(alpha, networkEndpointGroup) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error converting NetworkEndpointGroup to compute alpha type via JSON: %v", err) |
There was a problem hiding this comment.
see comment above about using %T instead of writing out each object type
pkg/composite/gen.go
Outdated
| beta := &computebeta.NetworkEndpointGroup{} | ||
| err := copyViaJSON(beta, networkEndpointGroup) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error converting NetworkEndpointGroup to compute beta type via JSON: %v", err) |
There was a problem hiding this comment.
see comment above about using %T instead of writing out each object type
pkg/composite/gen.go
Outdated
| ga := &compute.NetworkEndpointGroupsAttachEndpointsRequest{} | ||
| err := copyViaJSON(ga, networkEndpointGroupsAttachEndpointsRequest) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error converting NetworkEndpointGroupsAttachEndpointsRequest to compute ga type via JSON: %v", err) |
There was a problem hiding this comment.
see comment above about using %T instead of writing out each object type
pkg/composite/gen.go
Outdated
| networkEndpointGroupsDetachEndpointsRequest := &NetworkEndpointGroupsDetachEndpointsRequest{} | ||
| err := copyViaJSON(networkEndpointGroupsDetachEndpointsRequest, obj) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not copy object %+v to NetworkEndpointGroupsDetachEndpointsRequest via JSON: %v", obj, err) |
There was a problem hiding this comment.
see comment above about using %T instead of writing out each object type
pkg/composite/gen.go
Outdated
| alpha := &computealpha.NetworkEndpointGroupsDetachEndpointsRequest{} | ||
| err := copyViaJSON(alpha, networkEndpointGroupsDetachEndpointsRequest) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error converting NetworkEndpointGroupsDetachEndpointsRequest to compute alpha type via JSON: %v", err) |
There was a problem hiding this comment.
see comment above about using %T instead of writing out each object type
pkg/composite/gen.go
Outdated
| beta := &computebeta.NetworkEndpointGroupsDetachEndpointsRequest{} | ||
| err := copyViaJSON(beta, networkEndpointGroupsDetachEndpointsRequest) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error converting NetworkEndpointGroupsDetachEndpointsRequest to compute beta type via JSON: %v", err) |
There was a problem hiding this comment.
see comment above about using %T instead of writing out each object type
pkg/composite/gen.go
Outdated
| ga := &compute.NetworkEndpointGroupsDetachEndpointsRequest{} | ||
| err := copyViaJSON(ga, networkEndpointGroupsDetachEndpointsRequest) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error converting NetworkEndpointGroupsDetachEndpointsRequest to compute ga type via JSON: %v", err) |
There was a problem hiding this comment.
see comment above about using %T instead of writing out each object type
|
looks pretty good, just a few cleanups |
prameshj
force-pushed
the
composite-zonal
branch
from
05c8073 to
7a4d70e
Compare
Thanks for the review, I have made the recommended changes. PTAL, thanks! |
Create "ServerResponse" field only for MainServices that require CRUD methods. Combined regional,global, zonal templates into one. Added comments
prameshj
force-pushed
the
composite-zonal
branch
from
7a4d70e to
af13535
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bowei, prameshj 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
Current usecase is for using NetworkEndpointGroups.