chore: tweak schema for forwardingRule -> memorystore service attachemnt#7635
chore: tweak schema for forwardingRule -> memorystore service attachemnt#7635justinsb wants to merge 2 commits intoGoogleCloudPlatform:masterfrom
Conversation
justinsb
force-pushed
the
memorystore_forwardingrule_target
branch
from
0a1506b to
a3d37b7
Compare
|
cc @suwandim /assign @cheftako /assign @ldanielmadariaga Looks like we have to create a separate refs package; it's kind of surprising we got away without it for this long. The identity & reference are just moved into a new package memorystorerefs, except that NewInstanceIdentity has a dependency on |
| import ( | ||
| computev1beta1 "github.com/GoogleCloudPlatform/k8s-config-connector/apis/compute/v1beta1" | ||
| refsv1beta1 "github.com/GoogleCloudPlatform/k8s-config-connector/apis/refs/v1beta1" | ||
| "github.com/GoogleCloudPlatform/k8s-config-connector/apis/memorystore/memorystorerefs" |
There was a problem hiding this comment.
Yes, we repeat "memorystore". I think it makes it much more ergonomic though; even if we called it "memorystore/refs" we'd probably still have to alias it to memorystorerefs (just as we alias the refs package currently)
| "strings" | ||
| ) | ||
|
|
||
| // InstanceIdentity defines the resource reference to MemorystoreInstance, which "External" field |
There was a problem hiding this comment.
Note that we clean up the identity and reference types to follow our latest patterns in #7621
justinsb
changed the title
justinsb
force-pushed
the
memorystore_forwardingrule_target
branch
from
a3d37b7 to
dc40a42
Compare
Also we finally hit a circular dependency between compute and memorystore, so we need to move the instance identity and ref into its own package, that both compute and memorystore can use without creating a circular dependency.
justinsb
force-pushed
the
memorystore_forwardingrule_target
branch
from
dc40a42 to
22a9b31
Compare
| @@ -0,0 +1,61 @@ | |||
| // Copyright 2025 Google LLC | |||
There was a problem hiding this comment.
Guessing this still needs to say 2026 even if the contents of this file are technically a branch
| } | ||
|
|
||
| // New builds a InstanceIdentity from the Config Connector Instance object. | ||
| func NewInstanceIdentity(ctx context.Context, reader client.Reader, obj *MemorystoreInstance) (*memorystorerefs.InstanceIdentity, error) { |
There was a problem hiding this comment.
Are we forced to keep this builder here instead of moving it to memorystorerefs? Even if that's the case seems preferable to just have a passthrough here and avoid the exported fields InstanceIdentity.
Passthrough:
NewInstance(...) {
return memorystorerefs.NewInstance(...)
}
There was a problem hiding this comment.
If we put it back in the same package we end up with circular dependencies again.
In #7621 we're going to export all the fields anyway. There's no reason to "hide" the Identity schema (IMO).
| // InstanceIdentity defines the resource reference to MemorystoreInstance, which "External" field | ||
| // holds the GCP identifier for the KRM object. | ||
| type InstanceIdentity struct { | ||
| Parent_ *InstanceParent |
There was a problem hiding this comment.
Weird naming scheme and having exported fields collides a bit with having getters. See my comment in apis/memorystore/v1beta1/instance_types.go
There was a problem hiding this comment.
Right - we could merge #7621 first if you'd prefer
There was a problem hiding this comment.
Not sure if this is intended, it seems in most cases we prefer to have a generate.sh per API version.
There was a problem hiding this comment.
It was intended: once we have refs separately it becomes more obvious that we only need one generate.sh per API version.
However, if we put the refs into apis/refs then I can put this back as it was before. We can always clean this up later.
| k8s.io/klog/v2 => k8s.io/klog/v2 v2.130.1 | ||
| k8s.io/kube-openapi => k8s.io/kube-openapi v0.0.0-20250318190949-c8a335a9a2ff | ||
| k8s.io/kubectl => k8s.io/kubectl v0.32.1 | ||
| sigs.k8s.io/structured-merge-diff/v6 => sigs.k8s.io/structured-merge-diff/v4 v4.6.0 |
There was a problem hiding this comment.
I don't know why we're pinning so much stuff here, but this alias is very wrong.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ldanielmadariaga The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
3 participants
Also we finally hit a circular dependency between compute and memorystore,
so we need to move the instance identity and ref into its own package,
that both compute and memorystore can use without creating a circular dependency.