feat: add ManagedKafkaTopic as a direct resource #3585
Conversation
|
/assign @jasonvigil |
| // connecting directly to the cluster. Structured like: | ||
| // projects/{project}/locations/{location}/clusters/{cluster}/topics/{topic} | ||
| // +kcc:proto:field=google.cloud.managedkafka.v1.Topic.name | ||
| Name *string `json:"name,omitempty"` |
There was a problem hiding this comment.
Is this the same as the externalRef?
There was a problem hiding this comment.
Yes, this is an output field that essentially is the same as externalRef. I can see that we might want to ignore it since it serves the same purpose. We just need to be consistent cross the resources.
There was a problem hiding this comment.
I have not been adding this field to observedState for my resources, since it is the same as externalRef.
There was a problem hiding this comment.
Sure, I'll remove it from this resource and also from "ManagedKafkaCluster."
There was a problem hiding this comment.
I added TODOs to remove the field in a followup PR.
| return mapCtx.Err() | ||
| } | ||
|
|
||
| // cannot use common.CompareProtoMessage because some proto fields are not correctly labeled as output only, which means the path may include a output only field |
There was a problem hiding this comment.
Which fields are incorrectly labeled? Seems the name field is the only output field. Reason I ask is because name is common to a lot of resources. Should we be treating name as an output field?
There was a problem hiding this comment.
IIRC, the name field is not explicitly labeled as "output only." This means the YAML does not include name in spec since it isn't a spec field, but the actual GCP resource has a computed name field. As a result, the function detects a diff in the name field.
However, for this resource, the name field is indeed an output-only field. In the Create API, the "topic_id" is required, while the name field is explicitly ignored.
Should we be treating name as an output field?
I believe it depends on the resource. If a resource ignores the "name" field during creation and computes its value afterward, then it should be considered an output-only field.
There was a problem hiding this comment.
As a result, the function detects a diff in the name field.
To work around this issue, I have been explicitly setting the name field in the proto object (after mapping ToProto), before comparing. Here is an example:
There was a problem hiding this comment.
Sound good. I will give it a try. Thanks!
|
/lgtm |
|
/assign yuwenma |
| commonv1alpha1.CommonSpec `json:",inline"` | ||
|
|
||
| // +required | ||
| Location string `json:"location"` |
There was a problem hiding this comment.
Suggest adding descriptions for Location and ClusterRef, because these comments are the source to generate the reference doc, which is the most straightforward (if not only) way for users to configure the KCC object
| // connecting directly to the cluster. Structured like: | ||
| // projects/{project}/locations/{location}/clusters/{cluster}/topics/{topic} | ||
| // +kcc:proto:field=google.cloud.managedkafka.v1.Topic.name | ||
| Name *string `json:"name,omitempty"` // TODO(jingyih): remove this field as it is the same as externalRef |
There was a problem hiding this comment.
Wait, any reasons to add this field (with a cleanup TODO) if this is a pure new Alpha resource?
There was a problem hiding this comment.
Yeah this is a new resource. I am removing the field in a followup PR.
|
/approve Left a few nits. Non blockers. Hold in case you can take a look |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yuwenma 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 |
jingyih
force-pushed
the
managed_kafka_topic
branch
from
c6056a0 to
32b1013
Compare
|
PR rebased to pick up the fix for broken HEAD. |
|
/lgtm |
|
/hold cancel |
google-oss-prow
bot
merged commit 2aa4fe3
into
GoogleCloudPlatform:master
No description provided.