builtin: Return evalerrors.ErrEvaluationSkipSilently in case the builtin evaluator doesn't match the entity #800
Conversation
jhrozek
force-pushed
the
builtin_skip_silent
branch
from
b0c1bc3
to
b9e9216
Compare
jhrozek
force-pushed
the
builtin_skip_silent
branch
from
b9e9216
to
5ff2fd5
Compare
| // GetMethod gets the method by name from the RuleMethods struct | ||
| func (r *RuleMethods) GetMethod(mName string) (reflect.Value, error) { | ||
| value := reflect.ValueOf(r) | ||
| method := value.MethodByName(mName) | ||
|
|
||
| // Check if the method exists | ||
| if !method.IsValid() { | ||
| return reflect.Value{}, fmt.Errorf("rule method not found") | ||
| } | ||
|
|
||
| return method, nil | ||
| } |
There was a problem hiding this comment.
Does this work, or do we just have no methods defined right now?
There was a problem hiding this comment.
currently there's no methods. We used to have the artifact verification method as a single builtin but the method and the builtin ingestor as a whole proved to be too limiting for what we needed to do so I ended up just adding a specialised artifact ingestor.
As I was trying to use the builtin ingestor for artifacts, I noticed this bug that we treat skipped entries as passing policy. That felt like a big enough bug to fix even if the ingestor is unused at the moment.
But at the same time, I think the builtin ingest still has value, so I didn't want to just nuke it completely. Actually, as I was writing this comment I realised that it would be better and make for easier testing if we have at least one method, so I added a "Passthrough" (that we already talked about before).
This also makes for better tests which are needed /especially/ because we don't use the builtin ingestor at the moment.
pkg/rule_methods/rule_methods.go
Outdated
| ) | ||
|
|
||
| // RuleMethodGetter is the interface that is used to get the method by name | ||
| type RuleMethodGetter interface { |
There was a problem hiding this comment.
What do you think of calling this Methods since it's already in the rule_methods module (vs rule_methods.RuleMethodGetter, which has a high character : meaning ratio).
| return reflect.Value{}, fmt.Errorf("rule method not found") | ||
| } | ||
|
|
||
| return method, nil |
There was a problem hiding this comment.
It would be super-nifty if this returned a typed result that could be invoked without the jiggery-pokery on lines 76-87 of builtin.go (i.e. by moving them here, but other RuleMethodGetter implementations could use actual types).
There was a problem hiding this comment.
I think that type would be:
func(context.Context, string, any) (any, error) or maybe func(context.Context, string, any) (json.RawMessage, error)
There was a problem hiding this comment.
I'm probably a bad Go programmer but I couldn't make the conversion from the reflected Value to the function type work.
jhrozek
force-pushed
the
builtin_skip_silent
branch
2 times, most recently
from
f7704fd
to
e434373
Compare
…tin evaluator doesn't match the entity Returning `nil, nil` is wrong as it would be treated the same as "evaluation was performed an no issues were found"
jhrozek
force-pushed
the
builtin_skip_silent
branch
from
e434373
to
792d3b3
Compare
|
@JAORMX I think your change is covered now, if you're ok to re-review. |
Returning
nil, nilis wrong as it would be treated the same as"evaluation was performed an no issues were found"