-
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
chore: generate types & mappers for RedisCluster #2289
chore: generate types & mappers for RedisCluster #2289
Conversation
62b3d97
to
fa9c6fa
Compare
"google.golang.org/protobuf/types/known/timestamppb" | ||
) | ||
|
||
func Cluster_CreateTime_FromProto(mapCtx *MapContext, in *timestamppb.Timestamp) *string { |
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 think we may be able to automatically generate these (or automatically generate them, then allow overrides) ... but I want to collect a few more examples first
fa9c6fa
to
0125ec2
Compare
@@ -755,8 +755,10 @@ func (v *visitor) writeMapFunctions() { | |||
v.generatedFiles[k] = out | |||
|
|||
out.contents.WriteString(fmt.Sprintf("package %s\n\n", lastGoComponent(goPackage))) | |||
out.contents.WriteString(fmt.Sprintf("import pb %q\n\n", "cloud.google.com/go/monitoring/dashboard/apiv1/dashboardpb")) | |||
out.contents.WriteString(fmt.Sprintf("import krm %q\n\n", "github.com/GoogleCloudPlatform/k8s-config-connector/apis/monitoring/v1beta1")) | |||
out.contents.WriteString("import (\n") |
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 is what gofmt does, saves us a gofmt (as do some of the tab changes)
{{- if .PackageProtoTag }} | ||
// +kcc:proto={{ .PackageProtoTag }} | ||
{{- end }} | ||
{{ if .PackageProtoTag }}// +kcc:proto={{ .PackageProtoTag }}{{ end }} |
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.
Avoiding an awkward (lack of) line break
return "", false | ||
} | ||
|
||
protoPackagePath = string(msg.ParentFile().Package()) |
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're now driven much more by the flags
@@ -93,7 +93,7 @@ func RunGenerateMapper(ctx context.Context, o *GenerateMapperOptions) error { | |||
if strings.HasSuffix(fullName, "Response") { | |||
return "", false | |||
} | |||
if !strings.HasPrefix(fullName, o.ServiceName) { | |||
if !strings.HasPrefix(fullName, o.ServiceName+".") { |
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.
Avoids foo.v1alpha1 matching foo.v1 !
@@ -499,7 +509,12 @@ func (v *MapperGenerator) writeMapFunctionsForPair(out io.Writer, pair *typePair | |||
} | |||
|
|||
oneof := protoField.ContainingOneof() | |||
if oneof != nil { | |||
if protoField.HasOptionalKeyword() { |
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.
A lot of this complexity is because we are guessing the go field types. It might easier to actually look at the target go type, and it might also save us some overrides!
out.contents.WriteString("import (\n") | ||
out.contents.WriteString(fmt.Sprintf("\tpb %q\n", pbPackage)) | ||
out.contents.WriteString(fmt.Sprintf("\tkrm %q\n", krmPackage)) | ||
out.contents.WriteString(fmt.Sprintf("\t%q\n", "github.com/GoogleCloudPlatform/k8s-config-connector/pkg/controller/direct")) |
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.
All this direct stuff is using the code that @acpana just refactored out 🎉
if count > 4 { | ||
return | ||
} | ||
// Generate a "reasonable" timestamp |
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.
"Unreasonable" dates are (literally) billions of years in the future. Not a problem in practice, but the fuzzer finds these problems pretty fast if we don't stop them!
) | ||
|
||
// Status fields | ||
unimplementedFields.Insert(".discovery_endpoints") |
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 file is not currently generated. I think it would be nice to collect more examples and then automate. e.g. I think we can tell which fields are spec and which are status by seeing which ones map to spec and which to status (and if they map to neither, that's an error)
"google.golang.org/protobuf/types/known/timestamppb" | ||
) | ||
|
||
func Cluster_CreateTime_FromProto(mapCtx *direct.MapContext, in *timestamppb.Timestamp) *string { |
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 should be able to automatically generate these in future (particularly once we're able to skip generation if there is a manually-written function with the same name). But I want to collect more examples first.
4d095ef
to
acd5abd
Compare
acd5abd
to
47ef09e
Compare
out.contents.WriteString("import (\n") | ||
out.contents.WriteString(fmt.Sprintf("\tpb %q\n", pbPackage)) | ||
out.contents.WriteString(fmt.Sprintf("\tkrm %q\n", krmPackage)) | ||
out.contents.WriteString(fmt.Sprintf("\t%q\n", "github.com/GoogleCloudPlatform/k8s-config-connector/pkg/controller/direct")) |
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.
Why is this a string we need to manually validate? Wouldn't it be better to use direct.GoPackage ?
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 sure I follow - did this comment get moved between lines?
@@ -1,7 +1,6 @@ | |||
package apis | |||
|
|||
const DocTemplate = ` | |||
// Copyright 2024 Google LLC | |||
const DocTemplate = `// Copyright 2024 Google LLC |
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.
Would it make sense to construct DocTemplate and GroupVersionInfoTemplate from a new CopyRightTemplate? This both allows us to only fix the copyright in 1 place. It also emphasizes what is different and what is the same between them.
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.
Yeah, we can also automatically add the copyright to the file as part of writing it ... I think that's in a PR that's coming!
/lgtm Flagged a couple of spots I think we may want to improve but nothing which needs to hold this up. |
[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 |
6df3bad
into
GoogleCloudPlatform:master
This is (mostly) generated, and then we pull out a few types we want to override.
There is some duplicate generation of functions and types, detecting that and skipping duplicates that is in a separate branch/PR.