Refactor LfsImportService and ImportService
What does this MR do?
In https://gitlab.com/gitlab-org/gitlab-ee/issues/10871 we are going to reuse the service that import Lfs Objects, but ImportService has too much logic about it. In this MR we refactor the code to make it more straightforward and reusable.
Does this MR meet the acceptance criteria?
Conformity
Merge request reports
Activity
changed milestone to %11.11
added Create [DEPRECATED] DEPRECATED_import backend backstage [DEPRECATED] + 1 deleted label
marked the checklist item Separation of EE specific content as completed
marked the checklist item Style guides as completed
marked the checklist item Database guides as completed
marked the checklist item Merge request performance guidelines as completed
marked the checklist item Code review guidelines as completed
added 1 commit
- 95da825a - Refactored LfsImportService and ImportService
1 Warning 
This merge request is quite big (more than 560 lines changed), please consider splitting it into multiple merge requests. Reviewer roulette
Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.
To spread load more evenly across eligible reviewers, Danger has randomly picked a candidate for each review slot. Feel free to override this selection if you think someone else would be better-suited, or the chosen person is unavailable.
Once you've decided who will review this merge request, mention them as you normally would! Danger does not (yet?) automatically notify them for you.
Category Reviewer Maintainer backend Lucas Charles ( @theoretick)Jen-Shin Lin ( @godfat)Generated by
Edited by 🤖 GitLab Bot 🤖
@reprazent this is a refactor I need for https://gitlab.com/gitlab-org/gitlab-ee/issues/10871. Do you want to take the first review?
assigned to @reprazent
@fjsanpedro Some thoughts, I realize this is just moving code around to be able to reuse some stuff, but maybe we should clean up a bit while were there?
assigned to @fjsanpedro
added 1 commit
- 54b73772 - Refactored LfsImportService and ImportService
added 87 commits
-
54b73772...de44f3e1 - 86 commits from branch
master - efd5a903 - Refactored LfsImportService and ImportService
-
54b73772...de44f3e1 - 86 commits from branch
assigned to @reprazent
added 7 commits
-
efd5a903...c59f68e8 - 6 commits from branch
master - 4ad5b5f3 - Refactored LfsImportService and ImportService
-
efd5a903...c59f68e8 - 6 commits from branch
-
@fjsanpedro Some more suggestions, the main one being https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/27255#note_161611008, let's leave the bigger refactor for https://gitlab.com/gitlab-org/gitlab-ce/issues/60467 but I think we could tear apart the methods somewhat to have a tiny step in that direction, WDYT?
assigned to @fjsanpedro
mentioned in issue gitlab-com/www-gitlab-com#3350 (closed)
added 506 commits
-
65b22222...0ddc0a3f - 504 commits from branch
master - d3d5405e - Refactored LfsImportService and ImportService
- 350484cc - Code review comments applied
-
65b22222...0ddc0a3f - 504 commits from branch
-
@reprazent back to you again
Edited by Francisco Javier López assigned to @reprazent
-
Cool @fjsanpedro! Just some suggestions regarding the naming of stuff, though I'm not super convinced about my suggestions either
I think this is already a nice step forward for https://gitlab.com/gitlab-org/gitlab-ce/issues/60467
assigned to @fjsanpedro
mentioned in issue #60467 (moved)
-
@reprazent I will leave the renamings to https://gitlab.com/gitlab-org/gitlab-ce/issues/60467. I don't want to introduce any new bug here. Thanks for your review
@godfat do you mind to take a look?
assigned to @godfat
@fjsanpedro Thanks! Sorry since I am off til Wednesday, so I only took a quick glimpse through it, and had one minor thought, which is not really important either.
Because of this, and @reprazent should be becoming as a maintainer very soon, I would like to just rely on his judgement.
@reprazent I see you don't have the maintainer privilege yet, but I can hit the button for you in this case. Let me know if you want me to merge it.
assigned to @reprazent
-
@godfat Sorry, I didn't notice you were off. I've reviewed this as a first review, but it still needs a second pair of eyes
@jameslopez would you have a moment to be that second pair of eyes?
assigned to @jameslopez
@reprazent No problems. I was assigned before I was off, but I didn't have time and I decided to take a look on Monday, but then I see this might take some time so I decided to not looking into it too carefully for now
-
Thanks @fjsanpedro - left some suggestions :)
assigned to @fjsanpedro
assigned to @jameslopez
-
Merging this, we can tackle the tech debt items in https://gitlab.com/gitlab-org/gitlab-ce/issues/60467
Thanks @fjsanpedro!
mentioned in commit ee1b6d31
mentioned in issue gitlab-org/release/tasks#778 (closed)
added devopscreate label
added typemaintenance label
added typefeature label
removed typefeature label
added Category:Source Code Management label and removed 1 deleted label
