google / cel-spec

Common Expression Language -- specification and binary representation
https://cel.dev
Apache License 2.0
2.67k stars 216 forks source link

Add canonical CEL Expr Protos #266

Closed l46kok closed 1 year ago

l46kok commented 1 year ago

These represent the unversioned, canonical CEL expression protos. These definitions are the same as google.api.expr.v1alpha1 protos, except for the package name that was changed to dev.cel.expr.

TristonianJones commented 1 year ago

/gcbrun

l46kok commented 1 year ago

PTAL

l46kok commented 1 year ago

/gcbrun

jhump commented 1 year ago

@TristonianJones, shouldn't this folder be properly namespaced? In other words, these should be in proto/dev/cel/expr instead of proto/expr. That way the "canonical" file name and import path for these files will have "dev/cel/expr" path, just as the previous alpha and beta protos (over in googleapis repo) are in a directory named google/api/expr, to match their declared package.

Having folders/paths match the package is a typical best practice with protos and helps to prevent issues in runtimes where that "canonical" path is part of the file's identity (such as Go and C++); in those runtimes, using an unqualified path could lead to inadvertent conflicts with users' protos that may lead to a user's program crashing during initialization.

jhump commented 1 year ago

BTW, I'd like to bring these canonical protos into Buf, so they can be used instead of the alpha and beta versions (which are already in the Buf Schema Registry). But I think proper canonical paths must be settled first before people can reliably use them.

TristonianJones commented 1 year ago

@l46kok seems reasonable to me, what do you think?

l46kok commented 1 year ago

I agree, I'll send out a PR

l46kok commented 1 year ago

PR #273 should address this. Thanks for letting us know @jhump

jhump commented 1 year ago

@l46kok, thanks!!