Fix error in Mapper::Normalizer when CI include value type is invalid
What does this MR do and why?
In #425139 (closed), we logged a backtrace that indicated an error:
undefined method `deep_symbolize_keys' for 123:Integer
This error occurs when expanding CI includes at Config::External::Mapper::Normalizer::29.
It can be reproduced by specifying an invalid value type for the include:
keyword like so:
include: 123 # Also fails when this is a Boolean
It also errors when you include an array of locations like so:
include:
- 123 # Also fails when this is a Boolean or Array
This MR fixes this problem by adding type validation on the location value. It should only be either a String or Hash type.
Notes about the approach:
- You might be wondering why this validation isn't already covered by
validates :config, hash_or_string: true
in Ci::Config::Entry::Include. This is because included files are expanded and mostly validated before@root.compose!
runs inCi::Config
. - Some of the validations in
Ci::Config::Entry::Include
do get executed, but that's only in certain scenarios where theinclude
keyword "falls through" to the@root.compose!
step. This can happen if it's not removed byCi::Config::External::Processor#remove_include_keyword!
. I think this is why we haven't just removed these Entry classes yet. - For the reasons above, it seems the most appropriate to add the validation logic to
Ci::Config::External::Mapper::Normalizer
. Also, it:- Aligns with the notion of "normalizing"
- Is one of the first steps in the file expansion process
- Is directly dependent on the value type being a string or hash
Resolves #425139 (closed).
MR acceptance checklist
Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
Screenshots
Before fix | After fix |
---|---|
![]() |
![]() |
How to set up and validate locally
- Update your project's
.gitlab-ci.yml
file with:
include: 123
- Observe that it outputs a validation error message as seen under Screenshots.
Related to #425139 (closed)
Edited by Leaminn Ma