Skip to content
This repository was archived by the owner on Jun 6, 2024. It is now read-only.

feat: add custom key for cel expression support #961

Merged
merged 11 commits into from
Jul 20, 2023

Conversation

TzlilSwimmer123
Copy link
Contributor

@TzlilSwimmer123 TzlilSwimmer123 commented Jul 18, 2023

Screen Shot 2023-07-19 at 15 09 31

@TzlilSwimmer123 TzlilSwimmer123 requested a review from a team as a code owner July 18, 2023 09:11
shmu3l
shmu3l previously approved these changes Jul 19, 2023
return ctx.Error(CustomKeyValidationErrorKeyPath, err.Error())
}
// wrap dataValue (the resource that should be validated) inside a struct with parent object key
dataValueObjectWrapper := make(map[string]interface{})
Copy link
Contributor

Choose a reason for hiding this comment

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

naming maybe resourceWithParentKey ?

}

func getCELEnvInputs(dataValue map[string]interface{}) ([]cel.EnvOption, error) {
var inputs map[string]any
Copy link
Contributor

Choose a reason for hiding this comment

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

var inputs map[string]interface{}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that it suppose to be any because it can be any type

Copy link
Contributor

Choose a reason for hiding this comment

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

any is an alias for interface{} :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know:)

royhadad
royhadad previously approved these changes Jul 19, 2023
@@ -0,0 +1,140 @@
// This file defines a custom key to implement the logic for rego rule:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// This file defines a custom key to implement the logic for rego rule:
// This file defines a custom key to implement the logic for a CEL rule:

if customKeyCELRule, ok := m[CELDefinitionCustomKey]; ok {
customKeyCELRuleObj, validObject := customKeyCELRule.([]interface{})
if !validObject {
return nil, fmt.Errorf("CELDefinition must be an object")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return nil, fmt.Errorf("CELDefinition must be an object")
return nil, fmt.Errorf("CELDefinition must be an array")

return ctx.Error(CustomKeyValidationErrorKeyPath, "cel expression needs to return a boolean")
}
if !celReturnValue {
return ctx.Error(CustomKeyValidationErrorKeyPath, "cel expression failure message: %s", celExpression.Message)
Copy link
Contributor

Choose a reason for hiding this comment

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

"cel expression failure message:"
does @adifayer want this prefix for every CEL failure? how do users experience it?
maybe include a screenshot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will show it to Adi

var CELDefinition CELDefinition
for _, CELExpressionFromSchema := range CELDefinitionSchema {
var CELExpression CELExpression
b, err := json.Marshal(CELExpressionFromSchema)
Copy link
Contributor

Choose a reason for hiding this comment

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

why Marshal and then Unmarshal? if you just need a type conversion then use Go's type conversion. Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok let's see, what is your suggestion?

func getCELEnvInputs(dataValue map[string]interface{}) ([]cel.EnvOption, error) {
var inputs map[string]any
dataValueBytes, _ := json.Marshal(dataValue)
if err := yaml.Unmarshal(dataValueBytes, &inputs); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's see if I understand this properly.

  • CEL expects the input to be a yaml
  • you get it as a Go object
  • therefore, you need to convert the object to a yaml
    is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your comment made me understand it was redundant! thanks

@TzlilSwimmer123 TzlilSwimmer123 merged commit 68a198d into main Jul 20, 2023
@delete-merged-branch delete-merged-branch bot deleted the CEL_expression_support branch July 20, 2023 13:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants