-
Notifications
You must be signed in to change notification settings - Fork 211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Generate CRDs & mappers for BigtableInstance #2288
Generate CRDs & mappers for BigtableInstance #2288
Conversation
justinsb
commented
Jul 13, 2024
- chore: generate CRDs for BigtableInstance from protos
- chore: generate mappers for bigtable
db8622b
to
eb2318e
Compare
*/ | ||
} | ||
|
||
/* NOTYET |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not yet, when?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So typically we / I have started doing one phase where we get the CRD yaml to match the existing CRD yaml. It's not a perfect match, the descriptions aren't exact, and we now mark status.observedGeneration as an int64. But there should be no field changes at this stage (we aren't using a new controller yet)
NOTYET is how I "stash" the fields that are going to be useful as we update the CRD, but we aren't yet adding support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this instance, we aren't introducing observedState yet, because it's an existing v1beta1 CRD. The only change we know we need right now for BigtableInstance is around the problems on autoscaling vs numNodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not against having a TODO/NOTYET but I do think we should communicate when we would want to include it.
If we want other people to contribute, having sign posts which indicate if this fields is ready to be exposed (or not) will help.
It can indicate its time or that not to waste your effort yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any suggestions? We at least want to wait till we have the direct controller. Maybe TODO-AFTER-DIRECT-CONTROLLER
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I'd even be happy with something that said something like we want the direct controller to have a minimum 3 month burn in as a beta resource before we add fields the non direct controller did not support.
@@ -70,12 +73,15 @@ spec: | |||
cpuTarget: | |||
description: The target CPU utilization for autoscaling. | |||
Value must be between 10 and 80. | |||
format: int64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this being explicit or does this represent a change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh .. good call...
/hold
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it's a change, but I think it's a good change. We should not be using integer
in CRDs, we should be using int32 or int64. Here it's an int64 according to the proto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(And we'll validate that it doesn't need to be quoted in our tests)
dev/tools/proto-to-mapper/main.go
Outdated
@@ -268,6 +268,7 @@ func (v *visitor) writeTypes(out io.Writer, msg protoreflect.MessageDescriptor) | |||
goType = "map[string]int64" | |||
} else { | |||
fmt.Fprintf(out, "// TODO: map type %v %v\n", keyKind, valueKind) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to include field or goFieldName or jsonName here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call - will do!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
I think I need to add mappings for the Cluster also - thanks for spotting the int64 change. It led me to check how this field is mapped in the proto. /hold |
162d6b2
to
35cf679
Compare
Actually, I don't think it matters yet that we aren't handling the clusters, that needs to be done in the controller, not the mappers. /hold cancel |
@@ -0,0 +1,17 @@ | |||
Examples: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a bit off. It seems like its copied from the generate.sh file. Do we need to keep this up to date? What do I learn here that I wouldn't get from the script and if there is something maybe we should highlight it? If we want to explain to someone how to run a generate an individual resource, maybe we should package the script into individual resources to make that easy and then have a top level which runs them all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed - they should be in generate.sh, and we will DRY generate.sh as we add more types.
I feel like I keep deleting this file, it just won't go away! Deleted!
@@ -139,7 +139,7 @@ func scaffoldDocFile(path string, cArgs *apis.APIArgs) error { | |||
if err := WriteToFile(path, out.Bytes()); err != nil { | |||
return err | |||
} | |||
color.HiGreen("New file added %s!\n", path) | |||
color.HiGreen("New file added %s\n", path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest we use %q for paths. I don't expect spaces, new lines etc in them but better to be safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case I removed the !
so that it is clickable. Agreed though, fixed
} | ||
|
||
func BackupInfo_StartTime_FromProto(mapCtx *direct.MapContext, in *timestamppb.Timestamp) *string { | ||
mapCtx.NotImplemented() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a test to ensure that passing a StartTime in a big table resource does not cause an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a fuzz test that checks the reachable functions, yes. This one is not reachable, because the BackupInfo message is not reachable. @jingyih is adding reachability testing at the proto level in another PR, so we may be able to not generate these unreachable messages in future (and thus we wouldn't need these methods)
I also think when we have the code to not emit functions that are already defined, we can generate default versions of these functions. We just don't have enough data yet on what they should look like ... need to do more resources!
Some more work in the controllerbuilder for oneof and optional enums.
35cf679
to
8b11371
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheftako 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 |
378a294
into
GoogleCloudPlatform:master