Validation of Complex CI Variable Expressions

Summary

We introduced complex CI variable expressions with && and || operators in 12.0. One of the effects of this is that we no longer store a full expression grammar, and instead need to validate expressions of arbitrary size on the fly. Our current check is somewhat crass - we simply evaluate the entire expression once, catch and report any raised errors, and then evaluate it again to return the "validated" parse tree. This isn't a great design.

Improvements

We should decide if and when we want to do a syntactic validation of an expression - parsing out the expression into tokens and checking things like operators receiving the proper number of operands, no dangling / characters, unrecognized tokens, etc.

Following that, we should decide if and when we want to do a semantic validation of the expression - interpolating all the available variables and evaluating them to make sure they're operable. I think this has to happen at run-time because the variables can be changed externally at any time.

Right now we just do both of these things at once as a sanity check, but I think a more well-design approach could allow us to provide better feedback to users, i.e. "Missing operand for "&&" operator in variable expression for "rspec" job."

Risks

The new behavior was introduced as a breaking change behind a feature flag. As we're dealing with rules of validity, it might make sense to take on this refactor while the feature flag is in place, in the event of a regression. Alternatively, we might use a separate feature flag so if something breaks we don't have to completely take away customers' complex expressions.

Involved components

lib/gitlab/ci/pipeline/expression/**/*.rb

I'm considering this a backstage issue as it pertains directly to how we validate. The possible feature benefits (such as error reporting) are very welcome for discussion here, but I think should end up with their own issues for prioritization and development.

Edited Jul 14, 2020 by Jason Yavorsky
Assignee Loading
Time tracking Loading