Consider raising a generic exception in `ci_configuration/base_create_service.rb`
Overview
The following comments from !112193 (merged) should be addressed:
-
@ahmed.hemdan started a discussion: (+2 comments) Also, maybe this is out of scope here, but why are we raising a
Gitlab::Graphql::Errors::MutationError
inside a service class?🤔 -
@ahmed.hemdan started a discussion: (+2 comments) question: likely out of the scope of this merge request, but is there a reason why the exception raised here is
Gitlab::Graphql::Errors::MutationError
?🤔 In my opinion, perhpas a generic error like
Gitlab::Graphql::Errors::BaseError
would be suitable unless maybe the following line (671) is in fact mutating the project by creating the sast configuration for it, is that the case?
Problem
In 65cb4b4e, two exceptions of type Gitlab::Graphql::Errors::MutationError
were introduced in:
Using that exception in the former is okay, given that sast_ci_configuration
triggers the generation of SAST configuration.
The latter, however, is the base service class of some child services like:
And while those service classes are used by graphql
mutations like:
But, they are also used elsewhere, as is the case with Project::CreateService
class.
Therefore, there are chances that MutationError
is not very descriptive of the error or exception at hand.
Proposal
Given the error is because the repository is empty, perhaps using a more generic exception is better here, e.g.:
raise ValidationError, _(format('You must %s before using Security features.', docs_link.html_safe)).html_safe