CI variable expression conjunction/disjunction
What does this MR do?
The primary purpose of this merge request is to add support for
|| operators in the CI config. It is a subset of, and should be considered a replacement of MR 27554.
A necessary but breaking change is also made to the matching of Lexeme::Pattern. The scanner is intentionally designed to be greedy (spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb:33) and match the outermost
/ characters in a statement, but this breaks down if we allow for multiple regular expressions in a statement, e.g.:
$VAR1 =~ /this/ && $VAR2 =~ /that/
I've written new Lexer specs to reproduce the issue and verify that statements are being correctly tokenized. The original failures looked like this:
expected: ["$PRESENT_VARIABLE", "=~", "/my var/", "&&", "$EMPTY_VARIABLE", "=~", "/nope/"] got: ["$PRESENT_VARIABLE", "=~", "/my var/ && $EMPTY_VARIABLE =~ /nope/"]
It's not a problem with the conjuction or disjunction operators themselves, but the possibility of having two distinct regex pattern tokens in a single statement that raises this issue.
I'm hesitating to change this abruptly since there is explicit specification for it to work the way it does, and would like to get some feedback and/or context about how our Pattern matching works before we decide what to do. I'm not sure there's a way to implement this without changing the matching for Pattern, in which case we should discuss how to detect instances of this breaking logic and warn users.
The pipeline is currently failing around a single spec regarding the escaping of slashes inside regex patterns. I'm punting on that until we decide what to do about this generally.
@grzesiek: You seem to have written the spec about greedy pattern matching, could you shed some light on the design and what it would mean to change the matching?
@brendan: This will be a user-facing breaking change, so I'm not sure how we'd want to introduce it.
I'm hoping that there's something I haven't thought of, but so far I think this is unavoidable.
Closes #62867 (closed)
Decide on what to do about intra-pattern
- Shore up the Lexer and Statement specs with lots of examples
- Tag and label this MR once we have an idea of if, how, and when this could be introduced
- Corresponding EE Merge Request: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/13956
Does this MR meet the acceptance criteria?
- Changelog entry
- Documentation created/updated or follow-up review issue created
- Code review guidelines
- Merge request performance guidelines
- Style guides
- Database guides
- Separation of EE specific content
Performance and testing
- Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
- Tested in all supported browsers
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
Label as security and @ mention
- The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
- Security reports checked/validated by a reviewer from the AppSec team