feat(cel): Add CEL custom library for semver comparison #202
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dinhxuanvu 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 |
openshift-ci
bot
added
the
approved
Add CEL custom library for semver comparison to api repo. This library is used in olm and registry. Signed-off-by: Vu Dinh <[email protected]>
pkg/cel/cel.go
Outdated
| type Evaluator interface { | ||
| Evaluate(env map[string]interface{}) (bool, error) | ||
| } | ||
|
|
||
| type EvaluatorProvider interface { | ||
| Evaluator(rule string) (Evaluator, error) | ||
| } |
There was a problem hiding this comment.
It would be good to get some GoDoc on these types to make it more obvious what their purpose is.
Are these specific to cel? If not, should they be in another package? Or should we name this package something else (e.g. constraint)?
There was a problem hiding this comment.
On the OLM PR, I call this package constraints but in api repo, I'm not sure if I should be more explicit about the constraint type. So I call it cel here but if constraints sounds more appropriate then I'm totally fine with that.
There was a problem hiding this comment.
Yep, if this package is renamed to constraint or constraints, it would make sense to me to have something like:
$ tree constraint
constraint
├── cel.go (cel-specific stuff here)
├── compound.go (All, Any, None constraints here)
└── constraint.go (common types and interfaces, e.g. Constraint and Evaluator*)
WDYT?
There was a problem hiding this comment.
Actually, looking more closely at the evaluator interface, it looks cel-specific (at least for now)?
I'm curious if there's a more generic constraint interface that all of the constraint types could implement? When @estroz goes to add the compound constraint struct, will there be an interface he can implement such that he doesn't have to change anything on the OLM side?
There was a problem hiding this comment.
The original code was meant to use with Evaluator() function below as a "selector" on which provider to use via id parameter (if you remember the original EP with evaluator struct with id field). Now that we don't have id part anymore, this provider becomes CEL-specific which is fine. If we introduce a new "provider" later, we can simply create a new NewSomethingEvaluatorProvider and add it to the SatResolver. Now evaluatorProvider on SatResolver is a field with a single provider but it can be a collection of providers. That shouldn't be a problem code-wise later on.
|
Since |
Remind the original cel package to constraints pkg to reflect that there will be more type of constraints. Remove the evaluator/provider interfaces that are not needed yet. The generic interface can be designed later when a new type of constraint is introduced. Signed-off-by: Vu Dinh <[email protected]>
|
After a discussion with Eric, it seems the generic interface design is not needed yet given only CEL is supported at the moment. There seems to be quite a bit of discussions about this interface that may need more time to figure out. For now, we can simply use struct and functions for CEL evaluation. |
|
I'm fine with how this PR stands now. We can always deprecate the types defined here once we have a more robust interface system designed to handle other constraint types as @joelanford has suggested here. |
|
I'm putting a hold here so people can approve and lgtm. |
openshift-ci
bot
added
the
do-not-merge/hold
|
/lgtm |
|
Ready for merge |
openshift-ci
bot
removed
the
do-not-merge/hold
Add CEL custom library for semver comparison to api repo. This
library is used in olm and registry.
Signed-off-by: Vu Dinh [email protected]