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.

Sign up for GitHub

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

Jump to bottom

feat(resolver): support generic constraint using CEL #2506

Merged
merged 4 commits into from
Dec 14, 2021
Merged

feat(resolver): support generic constraint using CEL #2506

merged 4 commits into from
Dec 14, 2021

Conversation

dinhxuanvu
Copy link
Member

@dinhxuanvu dinhxuanvu commented Dec 3, 2021 •
edited

Signed-off-by: Vu Dinh vudinh@outlook.com

Description of the change:

Introduce the new type of constraint that uses CEL (Common Expression Language)
as an expression language to define the constraint in a generic way.
This type of constraint will allow operator authors to specify a dependency
on any arbitrary properties in the bundle. This constraint also supports
logical operator such as AND and OR in the CEL expression.

Motivation for the change:

https://github.com/operator-framework/enhancements/blob/master/enhancements/generic-constraints.md

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /doc
  • Commit messages sensible and descriptive

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 3, 2021
@openshift-ci
Copy link

openshift-ci bot commented Dec 3, 2021

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 3, 2021
@dinhxuanvu dinhxuanvu force-pushed the generic-con branch 4 times, most recently from efce4b9 to f379574 Compare December 6, 2021 18:26
@dinhxuanvu dinhxuanvu changed the title [WIP] feat(resolver): support generic constraint using CEL feat(resolver): support generic constraint using CEL Dec 6, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 6, 2021
@dinhxuanvu dinhxuanvu added kind/feature Categorizes issue or PR as related to a new feature. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 6, 2021
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 7, 2021
@dinhxuanvu
Copy link
Member Author

Due to the automatic approved label added by the bot, I place a hold on here to avoid accident merging.
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 7, 2021
@dinhxuanvu dinhxuanvu force-pushed the generic-con branch 6 times, most recently from b798482 to 574293b Compare December 8, 2021 02:34
joelanford
joelanford reviewed Dec 8, 2021
View reviewed changes
pkg/controller/registry/resolver/cache/predicates.go Outdated
Comment on lines 335 to 337
evaluator constraints.Evaluator
rule string
message string
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'm being naive, but I'm a little surprised to see constraint-specific logic on the OLM side of this. I was imagining that we'd define a generic Constraint interface in o-f/api that Cel, All, Any, None, etc. would all implement.

That way, all OLM would need to do would be unmarshal the constraints and then use that generic interface to evaluate them. What am I missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is predicate information. The rule and message are there to surface those information to the resolver during failure with .String() function. It is not the same as Constraint struct in api repo.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 13, 2021
@estroz
Copy link
Member

estroz commented Dec 14, 2021

It would be nice to refactor some of this into a more generic setup like in #2418 but I can always rebase my PR if it goes in after this one, or do a follow-up if this goes in last.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2021
@dinhxuanvu
feat(resolver): support generic constraint using CEL
59d2d86 
Introduce the new type of constraint that uses CEL (Common Expression Language)
as an expression language to define the constraint in a generic way.
This type of constraint will allow operator authors to specify a dependency
on any arbitrary properties in the bundle. This constraint also supports
logical operator such as AND and OR in the CEL expression.

Signed-off-by: Vu Dinh <vudinh@outlook.com>
@dinhxuanvu
Add unit test for generic constraint and add some context on CEL library
4e82ce0 
Add unit test cases for generic constraint using CEL to ensure it works
properly in the resolver.

Add some context for the custom library for semver comparison in CEL.

Signed-off-by: Vu Dinh <vudinh@outlook.com>
@dinhxuanvu
Use constraints library from api with simplified CEL library
eb8cfb7 
Signed-off-by: Vu Dinh <vudinh@outlook.com>
@dinhxuanvu
Rebase against master to use property converter interface
2b5313b 
Signed-off-by: Vu Dinh <vudinh@outlook.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2021
@dinhxuanvu
Copy link
Member Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 14, 2021
estroz
estroz reviewed Dec 14, 2021
View reviewed changes
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

Nice work @dinhxuanvu

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2021
@openshift-merge-robot openshift-merge-robot merged commit e3dfbc5 into operator-framework:master Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joelanford joelanford joelanford left review comments

@perdasilva perdasilva perdasilva left review comments

@estroz estroz estroz left review comments

@anik120 anik120 anik120 requested changes

@ecordell ecordell Awaiting requested review from ecordell

@gallettilance gallettilance Awaiting requested review from gallettilance

Assignees

@estroz estroz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dinhxuanvu @estroz @joelanford @perdasilva @anik120 @openshift-merge-robot