chore: create generated mappers and roundtrip test for Dashboard #1862
Conversation
| Descriptor() protoreflect.EnumDescriptor | ||
| } | ||
|
|
||
| func Enum_ToProto[U ProtoEnum](ctx *MapContext, in *string) U { |
There was a problem hiding this comment.
big nit: Going to leave it up to you but could we rename ctx to mapCtx or something that doesn't make me think of the context.Context one 😛
There was a problem hiding this comment.
A much better name, thank you!
|
|
||
| inValue := ValueOf(in) | ||
| if inValue == "" { | ||
| unspecifiedValue := U(int32(0)) // defaultU.New(protoreflect.EnumNumber(0)).(U) |
There was a problem hiding this comment.
for my own learning, here and everywhere else, why not int64?
There was a problem hiding this comment.
Hmmm, good question. It shouldn't matter in go, because constant values are untyped (or something). Removed the int32 entirely, and it seems to still work :-)
There was a problem hiding this comment.
(proto.EnumNumber is int32, so it is actually int32 at the proto level, though proto int64 vs int32 is largely academic anyway as they both use variable length encoding, I believe)
| // if in == nil { | ||
| // return nil | ||
| // } | ||
|
|
||
| // v := *in |
There was a problem hiding this comment.
| // if in == nil { | |
| // return nil | |
| // } | |
| // v := *in |
remove?
| if count > 4 { | ||
| return | ||
| } | ||
| seconds := rand.Intn(365 * 24 * 60 * 60) |
There was a problem hiding this comment.
do we need to seed rand for determinism ?
There was a problem hiding this comment.
Will rename to randStream. And we do, the first "real" line of FuzzFromProto
There was a problem hiding this comment.
This is 💯 ; I'm wondering 1) if we want this to be in the direct package or test package long term and 2) if we can iterate over al the resources we care about with the same core.
There was a problem hiding this comment.
Agree. I am just starting it here, but I think we want to move it out pretty fast into a shared library!
There was a problem hiding this comment.
Wait how do we generate this?
There was a problem hiding this comment.
Using #1613 ... the gotcha is that I don't think we are all agreed on whether we should generate it this way. I hope to achieve that consensus when we all meet and reconcile our approaches.
If you want to use the tool, feel free though! I think we're coming up with a way of doing it in this stream of PRs:
- Get mockgcp running to understand the API and make testing easier
- Add a few more tests and capture some golden output
- Add to the direct test script
- Use the generated types and make sure that the CRD doesn't change "too much" (massive pain: the description strings are reformatted, making the diff painful)
- Add the generated mapper functions and fuzz test to make sure we have coverage.
- Write the "core controller" using those generated types & mapper functions
- At some point switch the direct reconciler to be "always on"
It's a bit fuzzy right now, but this seems to be what's evolving.
There was a problem hiding this comment.
Thanks for this overview! Let's give this a go! 💯
There was a problem hiding this comment.
There's nothing "monitoring" about this right? Trying to be careful about creating yet another magic machine (YAMM) but do we want this to be used across direct package?
dev/tasks/generate-crds
Outdated
| @@ -24,7 +24,7 @@ cd ${REPO_ROOT} | |||
|
|
|||
| # Run controller-gen to generate CRDs | |||
| cd ${REPO_ROOT}/apis | |||
| go run sigs.k8s.io/controller-tools/cmd/controller-gen@v0.14.0 object crd:crdVersions=v1,allowDangerousTypes=true output:crd:artifacts:config=config/crd/ paths="./..." | |||
| go run sigs.k8s.io/controller-tools/cmd/controller-gen@master object crd:crdVersions=v1,allowDangerousTypes=true output:crd:artifacts:config=config/crd/ paths="./..." | |||
There was a problem hiding this comment.
Do we want to keep this pinned ?
There was a problem hiding this comment.
Yes, but we need a PR. I sent #1868 to add a TODO
|
I've realized this is based on #1857 |
justinsb
force-pushed
the
monitoring_dashboard_roundtrip
branch
from
0047906 to
44bb4d3
Compare
justinsb
force-pushed
the
monitoring_dashboard_roundtrip
branch
from
44bb4d3 to
9d7ab55
Compare
This allows us to see unsupported fields.
justinsb
force-pushed
the
monitoring_dashboard_roundtrip
branch
from
9d7ab55 to
3d4cb37
Compare
|
thanks for addressing comments 🥇 /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: acpana 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 b07f99c
into
GoogleCloudPlatform:master
| out.Widgets = Slice_ToProto(mapCtx, in.Widgets, Widget_ToProto) | ||
| return out | ||
| } | ||
|
|
There was a problem hiding this comment.
This looks really good.
Why do you comment out some lines? I saw they are NOTYET. is there any issues you hit?
| krm "github.com/GoogleCloudPlatform/k8s-config-connector/apis/monitoring/v1beta1" | ||
| ) | ||
|
|
||
| func Aggregation_FromProto(mapCtx *MapContext, in *pb.Aggregation) *krm.Aggregation { |
There was a problem hiding this comment.
I like this. I think we can also include the versions, i.e. Aggregation_v1beta1_FromProto_v1
| "k8s.io/klog/v2" | ||
| ) | ||
|
|
||
| // IDEA: Load all the samples, and check that we have all the KRM paths covered |
There was a problem hiding this comment.
+100. Love this direction.
This allows us to see unsupported fields.