Add field mapper for attached clusters direct controller migration. #2966
Conversation
|
/assign @yuwenma |
hankfreund
force-pushed
the
scifi_attached
branch
from
de546e3 to
7d23cef
Compare
| return out | ||
| } | ||
|
|
||
| func slice_FromProto_two[T, U any](mapCtx *direct.MapContext, in []T, mapper func(mapCtx *direct.MapContext, in T) *U) []U { |
There was a problem hiding this comment.
Is this because the current Slice_FromProto has a pointer in the mapping function? If so, I think it would be worth adding this in the direct package to be reused. That can happen as a follow on PR. Let's rename this to something like slice_FromProto for now if so.
There was a problem hiding this comment.
That is correct.
And I did intend to rename it because this name is silly, I just forgot. My mistake!
| @@ -26,8 +26,7 @@ var ContainerAttachedClusterGVK = GroupVersion.WithKind("ContainerAttachedCluste | |||
| // +kcc:proto=google.cloud.gkemulticloud.v1.AttachedCluster | |||
| type ContainerAttachedClusterSpec struct { | |||
| /* The ID of the project in which the resource belongs. If it is not provided, the provider project is used. */ | |||
There was a problem hiding this comment.
I think by making this a required field now we should update the docs:
| /* The ID of the project in which the resource belongs. If it is not provided, the provider project is used. */ | |
| /* The ID of the project in which the resource belongs. */ |
There was a problem hiding this comment.
It was always a required field and the docs were misleading. I have updated them.
| @@ -26,8 +26,7 @@ var ContainerAttachedClusterGVK = GroupVersion.WithKind("ContainerAttachedCluste | |||
| // +kcc:proto=google.cloud.gkemulticloud.v1.AttachedCluster | |||
| type ContainerAttachedClusterSpec struct { | |||
| /* The ID of the project in which the resource belongs. If it is not provided, the provider project is used. */ | |||
| // +optional | |||
| ProjectRef *refs.ProjectRef `json:"projectRef,omitempty"` | |||
| ProjectRef *refs.ProjectRef `json:"projectRef"` | |||
There was a problem hiding this comment.
Mind elaborating this change?
There was a problem hiding this comment.
In the existing tf-based resource, this field is required even though the comment implies it is optional.
In my previous PR I made this optional to match the comment, but you called this out as a change in behavior, so I said I'd fix it. This is just putting everything back the way it was.
There was a problem hiding this comment.
Sorry I didn't make it clear. I mean the removal of the omitempty. I'm wondering why you make this change (because this could be an interesting topic!)
hankfreund
force-pushed
the
scifi_attached
branch
from
7d23cef to
980cacb
Compare
|
[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 |
google-oss-prow
bot
merged commit 7575dea
into
GoogleCloudPlatform:master
Followed step 3 of the Direct Controller migration guide.
Manually implemented functions which the generator was unable to handle on its own.
Since this is a migration for an existing resource, some
NOTYETs are in the code for new, unimplemented fields.Fix the change that made the required
ProjectRefoptional in the previous PR.