Skip to content

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, like Transformers::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 the after_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

Availability and Testing

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

Merge request reports

Loading