feat(internal/sidekick/gcloud): add support for create commands#5890
feat(internal/sidekick/gcloud): add support for create commands#5890julieqiu wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements support for AIP-133 Create methods in the sidekick gcloud generator. It updates the ClientCall structure to handle resource body fields, maps scalar fields to CLI flags, and tracks skipped fields with TODO comments. The generator template was updated to produce the necessary Go code for both LRO and non-LRO Create operations. Feedback was provided regarding code duplication in the template logic for Create methods, suggesting a refactor to unify request construction.
julieqiu
force-pushed
the
add-gcloud-create-command
branch
2 times, most recently
from
d31ac8c to
c29b1aa
Compare
Add support for AIP-133 Create methods. The generated action composes the parent path, builds the resource body from CLI flags, calls the client method, waits for completion when the call is an LRO, and prints the returned resource via protojson. Enums, maps, repeated fields, nested messages, and fields are skipped with TODOs in the generated code and will be implemented in a follow up PR.
julieqiu
force-pushed
the
add-gcloud-create-command
branch
from
c29b1aa to
bd1c47d
Compare
coryan
left a comment
coryan
left a comment
There was a problem hiding this comment.
I don't like the TODOs in the generated output, but if you like them then 🤷 .
| {{.Name}}: cmd.{{.Kind}}("{{.Flag}}"), | ||
| {{- end}} | ||
| {{- range .ClientCall.BodySkippedFields}} | ||
| // TODO: {{.}} |
There was a problem hiding this comment.
It seems odd to output TODO: comments to the generated code? Nobody is going to edit that code to fix them. Maybe just put the TODOs in the generator?
| if hasBehavior(f, api.FieldBehaviorIdentifier) { | ||
| continue | ||
| } | ||
| if f.Map { |
There was a problem hiding this comment.
If anything, the TODOs belong here, no?
| } | ||
| kind, ok := scalarKind(f.Typez) | ||
| if !ok { | ||
| call.BodySkippedFields = append(call.BodySkippedFields, |
| t.Errorf("mismatch (-want +got):\n%s", diff) | ||
| gotCall, gotFlags := buildClientCall(test.method, test.model, test.goClient, test.hasPath) | ||
| if diff := cmp.Diff(test.want, gotCall); diff != "" { | ||
| t.Errorf("call mismatch (-want +got):\n%s", diff) |
There was a problem hiding this comment.
Something about howwewritego.md and the format of these errors.
| // ClientCall describes a Go client method invocation that should replace the | ||
| // default print-only action for a generated command. | ||
| type ClientCall struct { | ||
| // BodyAssignments lists the body's scalar fields that become CLI |
There was a problem hiding this comment.
Non-blocking: how are you planning to deal with fields that are deeply nested in the request "body"? For example:
Uses a "Secret" for the body, which (when using global secrets) requires this field to be set:
| switch t { | ||
| case api.TypezString: | ||
| return "String", true | ||
| case api.TypezInt32: |
There was a problem hiding this comment.
I assume the UInt* variations are not hard, nor Float and Double...
| continue | ||
| } | ||
| switch f.Typez { | ||
| case api.TypezEnum: |
There was a problem hiding this comment.
You are going to have fun with those.
coryan
left a comment
coryan
left a comment
There was a problem hiding this comment.
I don't like the TODOs in the generated output, but if you like them then 🤷 .
2 participants
Add support for AIP-133 Create methods. The generated action composes the parent path, builds the resource body from CLI flags, calls the client method, waits for completion when the call is an LRO, and prints the returned resource via protojson.
Enums, maps, repeated fields, nested messages, and fields are skipped with TODOs in the generated code for now.
For #5769