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

Allow configurable tag name overrides in native types #1009

Conversation

matthchr
Copy link
Contributor

Allowing tags other than "cel" enables more flexibility, particularly when the types in question aren't owned by those configuring the CEL environment.

The main use-case for this is enabling an experience in CEL that matches the shape of an object in JSON (or YAML). This includes:

  • Property casing: 'Property' in Go would likely be 'property' in JSON/YAML.
  • Embedded structures: metav1.ObjectType json:"metadata,omitempty" is common on Kuberentes resources, allowing users to request CEL use the JSON tags enables access via the ".metadata" property even though there is no such property on the actual Native type.

Reviews

  • Perform a self-review.
  • Make sure the Travis CI build passes.
  • Assign a reviewer once both the above have been completed.

Merging

  • If a CEL maintaner approves the change, it may be merged by the author if
    they have write access. Otherwise, the change will be merged by a maintainer.
  • Multiple commits should be squashed before merging.
  • Please append the line closes #<issue-num>: description in the merge message,
    if applicable.

Copy link

google-cla bot commented Aug 27, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@TristonianJones
Copy link
Collaborator

Hi @matthchr, thanks for the contribution. Would you mind signing the CLA? I'll try to give this a review by the end of the week.

/gcbrun

@matthchr
Copy link
Contributor Author

matthchr commented Aug 29, 2024

I've signed the CLA. Let me know if you have any questions as well - I'm happy to talk through our use-case. I had originally done a different approach, something like this:

type NativeFieldAdapter = func(rt reflect.Type, name string) (reflect.StructField, bool)

func defaultNativeFieldAdapter(rt reflect.Type, name string) (reflect.StructField, bool) {
	return rt.FieldByName(name)
}

that users could configure, before I realized that actually all I really needed was to be able to configure the tag to parse for the feature that already existed, because what I wanted to do was use JSON tags rather than a new CEL tag.

@matthchr matthchr force-pushed the feature/native-types-configuration-tag-to-parse branch from 5cb248c to 991d75d Compare September 5, 2024 23:56
@TristonianJones
Copy link
Collaborator

/gcbrun

Allowing tags other than "cel" enables more flexibility, particularly
when the types in question aren't owned by those configuring the CEL
environment.

The main use-case for this is enabling an experience in CEL that matches
the shape of an object in JSON (or YAML). This includes:
 - Property casing: 'Property' in Go would likely be 'property' in
   JSON/YAML.
 - Embedded structures: metav1.ObjectType `json:"metadata,omitempty"`
   is common on Kuberentes resources, allowing users to request CEL use
   the JSON tags enables access via the ".metadata" property even though
   there is no such property on the actual Native type.
@matthchr matthchr force-pushed the feature/native-types-configuration-tag-to-parse branch from 991d75d to f0b814d Compare September 6, 2024 16:10
@TristonianJones
Copy link
Collaborator

/gcbrun

@TristonianJones TristonianJones merged commit d561d0a into google:master Sep 9, 2024
2 checks passed
@matthchr matthchr deleted the feature/native-types-configuration-tag-to-parse branch September 10, 2024 21:45
@matthchr
Copy link
Contributor Author

Thanks for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants