Fix inconsistent naming for services that delete things

What does this MR do?

Fixes naming inconsistencies with services that delete things

Are there points in the code the reviewer needs to double check?

Why was this MR needed?

To keep code base cleaner and easier to navigate

What are the relevant issue numbers?

#20891 (closed)

Screenshots (if relevant)

Does this MR meet the acceptance criteria?

• CHANGELOG entry added
• Documentation created/updated
• API support added
• Tests
  • Added for this feature/bug
  • All builds are passing
• Conform by the style guides
• Branch has no merge conflicts with master (if you do - rebase it please)
• Squashed related commits together

@rspeicher can you take a look when you have time :) ?

Few notes I've seen:

• Naming of workers is also strange, in some cases is {Domain}{Action}Worker in some {Action}{Domain}Worker , maybe we could FIX this also ?
• Maybe we could group more services in namespaces, for example there is CreateBranchService and DeleteBranchService, could we move this into Branch::CreateService and Branch::DeleteService

Added 1 commit:

• ca5c2985 - Fix inconsistent naming for services that delete things

@rspeicher can't get specs to work I have problem with merge_request update_service, I think same bug as here #19970 (moved).

I think I didn't introduce this failure, since also on my master branch I got same faliure, wdyt?

Naming of workers is also strange, in some cases is {Domain}{Action}Worker in some {Action}{Domain}Worker , maybe we could FIX this also?

Maybe we could group more services in namespaces, for example there is CreateBranchService and DeleteBranchService, could we move this into Branch::CreateService and Branch::DeleteService

@dixpac Good observations. These should each be their own MRs, probably. I'm not sure we need to worry about the second one just yet, though.

Added 86 commits:

• ca5c2985...5a4ecb98 - 85 commits from branch gitlab-org:master
• 93934a1f - Fix inconsistent naming for services that delete things

@rspeicher I've fixed the problems :)

Added 96 commits:

• 93934a1f...d1da2e81 - 95 commits from branch gitlab-org:master
• a6562e83 - Fix inconsistent naming for services that delete things

@rspeicher are we ready to merge this :) ?

@dixpac We're approaching the 8.11 release and since this isn't a critical change, I'm more comfortable having it wait until after 8.11 is released. But we'll get it in, thanks!

Reassigned to @rspeicher

Milestone changed to %8.12

Added technical debt label

Added 1506 commits:

• a6562e83...beb0b666 - 1505 commits from branch gitlab-org:master
• c3f36147 - Fix inconsistent naming for services that delete things

Milestone changed to %8.13

Added 1085 commits:

• c3f36147...ab496d82 - 1084 commits from branch gitlab-org:master
• 0be88d44 - Fix inconsistent naming for services that delete things

Added ~164274 label

@dixpac Looks good, just need to fix the changelog and we can get it merged today.

Reassigned to @dixpac

Added 31 commits:

• 0be88d44...7b42ff63 - 30 commits from branch gitlab-org:master
• 7f7be465 - Fix inconsistent naming for services that delete things

@rspeicher thx I've fixed it :)

Reassigned to @rspeicher

Enabled an automatic merge when the build for 7f7be465 succeeds

@dixpac Set to merge, thanks!

@rspeicher thank you :).....do you want me to do anything about
Naming of workers is also strange, in some cases is {Domain}{Action}Worker in some {Action}{Domain}Worker , maybe we could FIX this also ?

@dixpac Do we have an issue to discuss that already? Feel free to open one and ping me.

@dixpac I guess this had a conflict by the time the auto-merge came around

Can you fix those and push please?

Reassigned to @dixpac

Resolved all discussions

@smcgivern just pushed rebased version to fork, but for some reason MR is not picking it https://gitlab.com/dixpac/gitlab-ce/commits/rename_delete_services

Added 1503 commits:

• 7f7be465...f0c7e671 - 1502 commits from branch gitlab-org:master
• af8bdcc4 - Fix inconsistent naming for services that delete things

Compare with previous version

@dixpac thanks, it looks like the worker just took a while to pick that up. Two things:

1. We're now at the point where we're only merging critical things to 8.13. I'm sorry about this, but could you move the CHANGELOG entry to 8.14 please?
2. Are we at all concerned that the class names for the workers are stored in the Sidekiq queues? That means that if a job has been scheduled under the old name, it will now never be picked up. /cc @rspeicher for that too, I'm not sure if this was already covered.

Hey @smcgivern ! Good question about sidekiq :), IMHO there are 3 options to handle this

• Send SIGUSR1 to Sidekiq daemon to tell it to stop accepting new work, let it finish running all the jobs in the queue, redeploy code with these changes, send SIGTERM to kill the daemon, and restart it
• Leave old class names for some time and delete them in future releases :)
• I can just get back old worker names (easiest :) )

@dixpac we could also migrate the worker names, but that would require downtime: see https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7006. This is a slight variation on your option 1. I'd be happy with taking back the old worker names - it's still slightly better than where we were before!

Added 1225 commits:

• af8bdcc4...6eeff67c - 1224 commits from branch gitlab-org:master
• c7956885 - Fix inconsistent naming for services that delete things

Compare with previous version

I reverted back to the original worker names, because I think that it is not worth having downtime just for the name change :), if you think that option 1 is better choice just ping me... no probs :)

I think that's up to @rspeicher! I think we can do it if we have downtime in a release anyway, but it's a pretty meh reason to do it.

Reassigned to @rspeicher

Let's avoid the downtime for now. This is already an improvement, and the referenced issue is specifically about services, not workers.

Thanks!

Milestone removed

@dixpac Any chance that you finish this one? Or should we just take over?

added ~889916 label

removed assignee

@rymai sorry I totally forgot about this one :) ...sure I will finish it no probs....I will try to push the changes during this week is that ok ? sorry one more time :)

Can you assign it to me ?

@dixpac Sure, thanks!

assigned to @dixpac

added 4162 commits

• c7956885...e2f0b830 - 4161 commits from branch gitlab-org:master
• a25ae359 - Fix inconsistent naming for services that delete things

Compare with previous version

added 1 commit

• 8d286dd6 - Fix inconsistent naming for services that delete things

Compare with previous version

@rymai @rspeicher @smcgivern I pushed the changes!

Note while resolving conflicts I noticed that I forgot to move destroy_user_service to services/users/destroy_service , so I've fixed that also

changed milestone to %8.17

assigned to @rymai

@dixpac Thanks, I had a few remarks but otherwise, LGTM!

assigned to @dixpac

@rymai thanks for the review. I will fix it over the weekend :)

added 700 commits

• 8d286dd6...b525aff6 - 699 commits from branch gitlab-org:master
• a954e9a2 - Fix inconsistent naming for services that delete things

Compare with previous version

added 1 commit

• db28e1ae - Fix inconsistent naming for services that delete things

Compare with previous version

resolved all discussions

@dixpac Thanks! Could you please retry the failing spec (or fix the failure if it's legit)?

@rymai yea I'm trying to fix it! Locally everything is green, but on CI update_service is failing, trying to figure out what is the problem. I will ping you when I resolve it, or if you can see something obvious scream :)

added 887 commits

• db28e1ae...a965edb8 - 885 commits from branch gitlab-org:master
• 6b9c327a - Fix inconsistent naming for services that delete things
• a6886896 - Remove chached service in DestroyGroup spec

Compare with previous version

added 1 commit

• 0dacf3c1 - Fix inconsistent naming for services that delete things

Compare with previous version

@rymai i manage to make the tests green. I had to revert back Groups::DesctoryService in spec not to use rspecs described_class. Because its used as shared_example when I use described_class in this specs other Groups specs start to fail. I suppose this was the reason why the original implementation didn't use described_class.

Sorry for waiting on this....I couldn't fix it faster due to my other obligations :)

changed milestone to %9.0

@dixpac Thanks a lot!

Sorry for waiting on this....I couldn't fix it faster due to my other obligations :)

Don't worry about that, thanks again!

assigned to @rymai

merged

mentioned in commit 8d443c01

@dixpac Hey, it seems that you missed Files::DeleteService! Would you like to take care of that? :)

@rymai ofc :).... how do you want me to push the changes...on the new MR https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9110 ?