tool: improve types generator #3667
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Fixes #3618 /assign @jasonvigil |
| @@ -23,6 +23,68 @@ import ( | |||
|
|
|||
| var AlloyDBClusterGVK = GroupVersion.WithKind("AlloyDBCluster") | |||
|
|
|||
| // +kcc:proto=google.cloud.alloydb.v1beta.Cluster.PscConfig | |||
| type Cluster_PSCConfig struct { | |||
There was a problem hiding this comment.
Hmmm, I assume the change is made because of the acronym capitalization issue? Can I get an /lgtm for #3624? With this change, you shouldn't need to do manual modifications.
There was a problem hiding this comment.
Right. I'm not sure if we reached a consensus on which acronym to include in that PR, so I manually moved the Go structs in this PR to unblock the fix.
|
/lgtm |
| @@ -120,6 +120,11 @@ func (g *TypeGenerator) WriteVisitedMessages() error { | |||
| continue | |||
| } | |||
|
|
|||
| if hasOnlyOutputFields(msg) { | |||
| klog.Infof("skipping generation of %q as it only contains output fields", msg.FullName()) | |||
There was a problem hiding this comment.
I see why you want to do this. But it will cause rabbit holes if the existence of the field itself is configurable. PAM entitlement has one case where a field is structured but has no sub fields . I believe this function will skip that field.
There was a problem hiding this comment.
My gut feeling is that this is going toward a magic machine. A lot of API logic is determined by the type generator, which itself determines the behavior based on best practice. I'm afraid this limits KCC ability to adjust the field in each resource level. e.g. the PAM entitlement exception.
To correctly generate Go types from proto, I think some kind of magic machine is necessary—either explicitly building one or using an LLM. However, I don’t believe LLMs are yet producing Go types at a high enough quality, meaning fixing the generated code might take more time than just using this tool. Therefore I think there is value in fixing the bugs we found in the tool so it can be used on most resources. For cases like the PAM entitlement you mentioned, we can decide whether to make the magic machine more complicated or just manually fix the generated code. |
ObservedStateGo struct.