WIP: BulkImports - Refactor Pipelines to use methods instead of `class_attributes`
What does this MR do?
My initial motivation for this refactoring was to reduce code indirection. With less indirection, IMHO, the code is easier to understand. After the initial refactoring, though, I saw that besides the optional indirection, not depending on the DSL makes it is easier to test and by using methods, instead of class_attributes
also enables the Pipeline to keep state between its steps, which might be useful to improve how we handle pagination.
Pros
-
Less indirection/Optional indirection:
The current DSL requires the Pipeline to delegate its steps to other objects, IMHO this might make the code reading harder for those with little knowledge of the
BulkImports
architecture. On the other hand, keeping most of the pipeline action within the pipeline itself means that one will have to open less files to understand what a pipeline does. Still, with the methods approach one can, optionally, extract long complex parts of a pipeline, likeTransformers::GroupAttributesTransformer
. -
Keeping pipeline state:
By converting the pipeline to an object one can keep state between the pipeline steps. This might be useful to improve the current pagination approach. For example, on the
extract
step the developer could save the pagination information to be checked on theafter_run
step. (possible follow-up) - Less DSL code: having less DSL code means less supporting code to maintain and test
- "Replace concerns with dedicated classes & composition" - I'm not sure if it fits, but by reducing the DSL we're reducing the complexity of a concern and moving it to classes, so I think it's related.
Cons
- By having all the transformations as one method, we won't be able to log each transformation individually.
Screenshots (strongly suggested)
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability 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 -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
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 @gitlab-com/gl-security/appsec
-
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
Edited by Kassio Borges