Remove Exception handling from Ci::YamlProcessor
Summary
Ci::Config currently rescues several Exception types and raises another Exception with the message of the first. Using Exceptions intentionally as flow control like this isn't a great idea, and as it exists we only return a single error message, even if multiple errors are detectable. We should run all of our available error detection in Config and return an array of error messages (which already exists to some extent) in the event that any things are wrong.
Improvements
Removing Exceptions from our application where possible makes our logic easier to read and understand. There are several level of raising and rescuing custom Exception types, passing the messages around that eventually make it back to the user. Consolidating our validation logic to one structure that more thoroughly checks for problems should make this code easier to work with.
Risks
YamlProcessor
on down through individual Config::Entry
subclasses all use a system of raising, rescuing, and re-raising. To refactor any part of it risks breaking other parts. I propose we scope this refactor to Exception handling beginning with Ci::YamlProcessor#initialize
, and following the call stack from processing a configuration file from there, replacing exceptions with returnable errors messages one layer at a time.
Involved components
app/controllers/projects/ci/lints_controller.rb
app/models/ci/pipeline.rb
lib/gitlab/ci/yaml_processor.rb
lib/gitlab/ci/config.rb
lib/gitlab/ci/config/extendable.rb
lib/gitlab/ci/config/extendable/entry.rb
lib/gitlab/ci/config/external/processor.rb
lib/gitlab/config/loader/yaml.rb
Plus related specs.