Skip to content
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

tools: require "--kind" and "--proto-resource" when generating types #2686

Merged

Conversation

jingyih
Copy link
Collaborator

@jingyih jingyih commented Sep 12, 2024

This PR introduces the following changes:

  • Marks the --proto-resource flag as explicitly required, since the proto resource name is required for identifying the starting point of type generation.
  • Ensures that the first character (rune) of the proto resource name is automatically capitalized within the tool, eliminating the need for users to remember to capitalize the first letter in their input.
  • Also marks the --kind flag as required, given that kind and proto message often do not have the same name.

@jingyih
Copy link
Collaborator Author

jingyih commented Sep 12, 2024

/assign @yuwenma

/cc @xiaoweim

Comment on lines 129 to 132
kind := o.ResourceKindName
if kind == "" {
kind = o.ResourceProtoName
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually my experience is that the Kind and resourceProtoName are always different, because Kind always has the service name prefixed, while resourceProtoName is always ignore that (which makes sense for both case due to the different conditions). So I think may be we shall make both --kind and --proto-resource as required to stabilize the output.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah makes sense. Updated

@jingyih jingyih changed the title tools: always capitalize first letter in proto message name tools: require --kind and --proto-resource when generating types Sep 16, 2024
@jingyih jingyih changed the title tools: require --kind and --proto-resource when generating types tools: require "--kind" and "--proto-resource" when generating types Sep 16, 2024
Comment on lines +80 to +86
if o.ResourceProtoName == "" {
return fmt.Errorf("`--proto-resource` is required")
}
if o.ResourceKindName == "" {
return fmt.Errorf("`--kind` is required")
}
o.ResourceProtoName = capitalizeFirstRune(o.ResourceProtoName)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if neither is provided previously? IIRC, if --kind and --proto-resource is not given, this tool generates everything except the "_types.go" file. Maybe we'd better still keep this functionality and only require both if either one is given.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I never tried running the command without --kind and --proto-resource. What is the intended behavior?

  • Prior to commit f5960e4, It looks like the tool will re-generate (a.k.a update) all generated types for the given GCP API. This function is already lost after the commit, the question is do we want to bring it back?
  • After commit f5960e4, we need to explicitly provide --kind, and the tool will re-generate (a.k.a. update) all generated types for the given kind.

Copy link
Collaborator

@yuwenma yuwenma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One suggestion, otherwise looks good!

Copy link
Collaborator

@yuwenma yuwenma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

We can discuss the flag usage further. But definitely not a blocker! Thanks for sending the improvements PR!

@google-oss-prow google-oss-prow bot added the lgtm label Sep 19, 2024
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit abe8356 into GoogleCloudPlatform:master Sep 19, 2024
16 checks passed
@yuwenma yuwenma added this to the 1.123 milestone Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants