Update docs to put prepend as the last line
This updates the documentation for adding EE specific features to clarify that `prepend` should be on the last line, and how to deal with code that depends on class methods redefined in EE. Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/53919
This commit is contained in:
Yorick Peterse
parent
9cebb1e490
commit
bf91b06b1d
1 changed files with 124 additions and 29 deletions
|
|
@ -119,10 +119,20 @@ This also applies to views.
|
|||
|
||||
### EE features based on CE features
|
||||
|
||||
For features that build on existing CE features, write a module in the
|
||||
`EE` namespace and `prepend` it in the CE class. This makes conflicts
|
||||
less likely to happen during CE to EE merges because only one line is
|
||||
added to the CE class - the `prepend` line.
|
||||
For features that build on existing CE features, write a module in the `EE`
|
||||
namespace and `prepend` it in the CE class, on the last line of the file that
|
||||
the class resides in. This makes conflicts less likely to happen during CE to EE
|
||||
merges because only one line is added to the CE class - the `prepend` line. For
|
||||
example, to prepend a module into the `User` class you would use the following
|
||||
approach:
|
||||
|
||||
```ruby
|
||||
class User < ActiveRecord::Base
|
||||
# ... lots of code here ...
|
||||
end
|
||||
|
||||
User.prepend(EE::User)
|
||||
```
|
||||
|
||||
Since the module would require an `EE` namespace, the file should also be
|
||||
put in an `ee/` sub-directory. For example, we want to extend the user model
|
||||
|
|
@ -231,7 +241,6 @@ the existing file:
|
|||
|
||||
```ruby
|
||||
class ApplicationController < ActionController::Base
|
||||
prepend EE::ApplicationController
|
||||
# ...
|
||||
|
||||
def after_sign_out_path_for(resource)
|
||||
|
|
@ -240,6 +249,8 @@ class ApplicationController < ActionController::Base
|
|||
|
||||
# ...
|
||||
end
|
||||
|
||||
ApplicationController.prepend(EE::ApplicationController)
|
||||
```
|
||||
|
||||
And create a new file in the `ee/` sub-directory with the altered
|
||||
|
|
@ -533,8 +544,6 @@ module API
|
|||
end
|
||||
end
|
||||
|
||||
prepend EE::API::MergeRequests
|
||||
|
||||
params :optional_params do
|
||||
# CE specific params go here...
|
||||
|
||||
|
|
@ -542,6 +551,8 @@ module API
|
|||
end
|
||||
end
|
||||
end
|
||||
|
||||
API::MergeRequests.prepend(EE::API::MergeRequests)
|
||||
```
|
||||
|
||||
And then we could override it in EE module:
|
||||
|
|
@ -582,10 +593,10 @@ module API
|
|||
authorize_read_builds!
|
||||
end
|
||||
end
|
||||
|
||||
prepend EE::API::JobArtifacts
|
||||
end
|
||||
end
|
||||
|
||||
API::JobArtifacts.prepend(EE::API::JobArtifacts)
|
||||
```
|
||||
|
||||
And then we can follow regular object-oriented practices to override it:
|
||||
|
|
@ -626,8 +637,6 @@ module API
|
|||
end
|
||||
end
|
||||
|
||||
prepend EE::API::MergeRequests
|
||||
|
||||
put ':id/merge_requests/:merge_request_iid/merge' do
|
||||
merge_request = find_project_merge_request(params[:merge_request_iid])
|
||||
|
||||
|
|
@ -639,6 +648,8 @@ module API
|
|||
end
|
||||
end
|
||||
end
|
||||
|
||||
API::MergeRequests.prepend(EE::API::MergeRequests)
|
||||
```
|
||||
|
||||
Note that `update_merge_request_ee` doesn't do anything in CE, but
|
||||
|
|
@ -676,27 +687,37 @@ or not we really need to extend it from EE. For now we're not using it much.
|
|||
|
||||
Sometimes we need to use different arguments for a particular API route, and we
|
||||
can't easily extend it with an EE module because Grape has different context in
|
||||
different blocks. In order to overcome this, we could use class methods from the
|
||||
API class.
|
||||
different blocks. In order to overcome this, we need to move the data to a class
|
||||
method that resides in a separate module or class. This allows us to extend that
|
||||
module or class before its data is used, without having to place a `prepend` in
|
||||
the middle of CE code.
|
||||
|
||||
For example, in one place we need to pass an extra argument to
|
||||
`at_least_one_of` so that the API could consider an EE-only argument as the
|
||||
least argument. This is not quite beautiful but it's working:
|
||||
least argument. We would approach this as follows:
|
||||
|
||||
```ruby
|
||||
# api/merge_requests/parameters.rb
|
||||
module API
|
||||
class MergeRequests < Grape::API
|
||||
def self.update_params_at_least_one_of
|
||||
%i[
|
||||
assignee_id
|
||||
description
|
||||
]
|
||||
module Parameters
|
||||
def self.update_params_at_least_one_of
|
||||
%i[
|
||||
assignee_id
|
||||
description
|
||||
]
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
prepend EE::API::MergeRequests
|
||||
API::MergeRequests::Parameters.prepend(EE::API::MergeRequests::Parameters)
|
||||
|
||||
# api/merge_requests.rb
|
||||
module API
|
||||
class MergeRequests < Grape::API
|
||||
params do
|
||||
at_least_one_of(*::API::MergeRequests.update_params_at_least_one_of)
|
||||
at_least_one_of(*Parameters.update_params_at_least_one_of)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
@ -708,16 +729,18 @@ And then we could easily extend that argument in the EE class method:
|
|||
module EE
|
||||
module API
|
||||
module MergeRequests
|
||||
extend ActiveSupport::Concern
|
||||
module Parameters
|
||||
extend ActiveSupport::Concern
|
||||
|
||||
class_methods do
|
||||
extend ::Gitlab::Utils::Override
|
||||
class_methods do
|
||||
extend ::Gitlab::Utils::Override
|
||||
|
||||
override :update_params_at_least_one_of
|
||||
def update_params_at_least_one_of
|
||||
super.push(*%i[
|
||||
squash
|
||||
])
|
||||
override :update_params_at_least_one_of
|
||||
def update_params_at_least_one_of
|
||||
super.push(*%i[
|
||||
squash
|
||||
])
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
@ -728,6 +751,78 @@ end
|
|||
It could be annoying if we need this for a lot of routes, but it might be the
|
||||
simplest solution right now.
|
||||
|
||||
This approach can also be used when models define validations that depend on
|
||||
class methods. For example:
|
||||
|
||||
```ruby
|
||||
# app/models/identity.rb
|
||||
class Identity < ActiveRecord::Base
|
||||
def self.uniqueness_scope
|
||||
[:provider]
|
||||
end
|
||||
|
||||
prepend EE::Identity
|
||||
|
||||
validates :extern_uid,
|
||||
allow_blank: true,
|
||||
uniqueness: { scope: uniqueness_scope, case_sensitive: false }
|
||||
end
|
||||
|
||||
# ee/app/models/ee/identity.rb
|
||||
module EE
|
||||
module Identity
|
||||
extend ActiveSupport::Concern
|
||||
|
||||
class_methods do
|
||||
extend ::Gitlab::Utils::Override
|
||||
|
||||
def uniqueness_scope
|
||||
[*super, :saml_provider_id]
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
```
|
||||
|
||||
Instead of taking this approach, we would refactor our code into the following:
|
||||
|
||||
```ruby
|
||||
# ee/app/models/ee/identity/uniqueness_scopes.rb
|
||||
module EE
|
||||
module Identity
|
||||
module UniquenessScopes
|
||||
extend ActiveSupport::Concern
|
||||
|
||||
class_methods do
|
||||
extend ::Gitlab::Utils::Override
|
||||
|
||||
def uniqueness_scope
|
||||
[*super, :saml_provider_id]
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
# app/models/identity/uniqueness_scopes.rb
|
||||
class Identity < ActiveRecord::Base
|
||||
module UniquenessScopes
|
||||
def self.uniqueness_scope
|
||||
[:provider]
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
Identity::UniquenessScopes.prepend(EE::Identity::UniquenessScopes)
|
||||
|
||||
# app/models/identity.rb
|
||||
class Identity < ActiveRecord::Base
|
||||
validates :extern_uid,
|
||||
allow_blank: true,
|
||||
uniqueness: { scope: Identity::UniquenessScopes.scopes, case_sensitive: false }
|
||||
end
|
||||
```
|
||||
|
||||
### Code in `spec/`
|
||||
|
||||
When you're testing EE-only features, avoid adding examples to the
|
||||
|
|
|
|||
Loading…
Reference in a new issue