summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAchilleas Pipinellis <axil@gitlab.com>2018-11-13 16:16:06 +0000
committerAchilleas Pipinellis <axil@gitlab.com>2018-11-13 16:16:06 +0000
commit42873a191f920820945ff97b4282231084dca429 (patch)
treeb6a61bbd0d8f0b984afa1f49ef81a55332a943d4
parent474ae41468f419b2de5a3493b61e69626c7218bb (diff)
parentbf91b06b1d5ebe95a82eccd98271ce47c781b3c5 (diff)
downloadgitlab-ce-42873a191f920820945ff97b4282231084dca429.tar.gz
Merge branch 'docs/update-prepend-documentation' into 'master'
Update docs to put prepend as the last line Closes #53919 See merge request gitlab-org/gitlab-ce!23017
-rw-r--r--doc/development/ee_features.md153
1 files changed, 124 insertions, 29 deletions
diff --git a/doc/development/ee_features.md b/doc/development/ee_features.md
index b6f053ff0e9..9aea03139ee 100644
--- a/doc/development/ee_features.md
+++ b/doc/development/ee_features.md
@@ -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