diff options
Diffstat (limited to 'doc/development/ee_features.md')
| -rw-r--r-- | doc/development/ee_features.md | 286 |
1 files changed, 285 insertions, 1 deletions
diff --git a/doc/development/ee_features.md b/doc/development/ee_features.md index fea92e740cb..287143d6255 100644 --- a/doc/development/ee_features.md +++ b/doc/development/ee_features.md @@ -33,6 +33,26 @@ rest of the code should be as close to the CE files as possible. [single code base]: https://gitlab.com/gitlab-org/gitlab-ee/issues/2952#note_41016454 +### EE-specific comments + +When complete separation can't be achieved with the `ee/` directory, you can wrap +code in EE specific comments to designate the difference from CE/EE and add +some context for someone resolving a conflict. + +```rb +# EE-specific start +stub_licensed_features(variable_environment_scope: true) +# EE specific end +``` + +```haml +-# EE-specific start += render 'ci/variables/environment_scope', form_field: form_field, variable: variable +-# EE-specific end +``` + +EE-specific comments should not be backported to CE. + ### Detection of EE-only files For each commit (except on `master`), the `ee-files-location-check` CI job tries @@ -350,6 +370,255 @@ class beneath the `EE` module just as you would normally. For example, if CE has LDAP classes in `lib/gitlab/ldap/` then you would place EE-specific LDAP classes in `ee/lib/ee/gitlab/ldap`. +### Code in `lib/api/` + +It can be very tricky to extend EE features by a single line of `prepend`, +and for each different [Grape](https://github.com/ruby-grape/grape) feature, +we might need different strategies to extend it. To apply different strategies +easily, we would use `extend ActiveSupport::Concern` in the EE module. + +Put the EE module files following +[EE features based on CE features](#ee-features-based-on-ce-features). + +#### EE API routes + +For EE API routes, we put them in a `prepended` block: + +``` ruby +module EE + module API + module MergeRequests + extend ActiveSupport::Concern + + prepended do + params do + requires :id, type: String, desc: 'The ID of a project' + end + resource :projects, requirements: ::API::API::PROJECT_ENDPOINT_REQUIREMENTS do + # ... + end + end + end + end +end +``` + +Note that due to namespace differences, we need to use the full qualifier for some +constants. + +#### EE params + +We can define `params` and utilize `use` in another `params` definition to +include params defined in EE. However, we need to define the "interface" first +in CE in order for EE to override it. We don't have to do this in other places +due to `prepend`, but Grape is complex internally and we couldn't easily do +that, so we'll follow regular object-oriented practices that we define the +interface first here. + +For example, suppose we have a few more optional params for EE, given this CE +API code: + +``` ruby +module API + class MergeRequests < Grape::API + # EE::API::MergeRequests would override the following helpers + helpers do + params :optional_params_ee do + end + end + + prepend EE::API::MergeRequests + + params :optional_params do + # CE specific params go here... + + use :optional_params_ee + end + end +end +``` + +And then we could override it in EE module: + +``` ruby +module EE + module API + module MergeRequests + extend ActiveSupport::Concern + + prepended do + helpers do + params :optional_params_ee do + # EE specific params go here... + end + end + end + end + end +end +``` + +This way, the only difference between CE and EE for that API file would be +`prepend EE::API::MergeRequests`. + +#### EE helpers + +To make it easy for an EE module to override the CE helpers, we need to define +those helpers we want to extend first. Try to do that immediately after the +class definition to make it easy and clear: + +``` ruby +module API + class JobArtifacts < Grape::API + # EE::API::JobArtifacts would override the following helpers + helpers do + def authorize_download_artifacts! + authorize_read_builds! + end + end + + prepend EE::API::JobArtifacts + end +end +``` + +And then we can follow regular object-oriented practices to override it: + +``` ruby +module EE + module API + module JobArtifacts + extend ActiveSupport::Concern + + prepended do + helpers do + def authorize_download_artifacts! + super + check_cross_project_pipelines_feature! + end + end + end + end + end +end +``` + +#### EE-specific behaviour + +Sometimes we need EE-specific behaviour in some of the APIs. Normally we could +use EE methods to override CE methods, however API routes are not methods and +therefore can't be simply overridden. We need to extract them into a standalone +method, or introduce some "hooks" where we could inject behavior in the CE +route. Something like this: + +``` ruby +module API + class MergeRequests < Grape::API + helpers do + # EE::API::MergeRequests would override the following helpers + def update_merge_request_ee(merge_request) + 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]) + + # ... + + update_merge_request_ee(merge_request) + + # ... + end + end +end +``` + +Note that `update_merge_request_ee` doesn't do anything in CE, but +then we could override it in EE: + +``` ruby +module EE + module API + module MergeRequests + extend ActiveSupport::Concern + + prepended do + helpers do + def update_merge_request_ee(merge_request) + # ... + end + end + end + end + end +end +``` + +#### EE `route_setting` + +It's very hard to extend this in an EE module, and this is simply storing +some meta-data for a particular route. Given that, we could simply leave the +EE `route_setting` in CE as it won't hurt and we are just not going to use +those meta-data in CE. + +We could revisit this policy when we're using `route_setting` more and whether +or not we really need to extend it from EE. For now we're not using it much. + +#### Utilizing class methods for setting up EE-specific data + +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. + +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: + +``` ruby +module API + class MergeRequests < Grape::API + def self.update_params_at_least_one_of + %i[ + assignee_id + description + ] + end + + prepend EE::API::MergeRequests + + params do + at_least_one_of(*::API::MergeRequests.update_params_at_least_one_of) + end + end +end +``` + +And then we could easily extend that argument in the EE class method: + +``` ruby +module EE + module API + module MergeRequests + extend ActiveSupport::Concern + + class_methods do + def update_params_at_least_one_of + super.push(*%i[ + squash + ]) + end + end + end + end +end +``` + +It could be annoying if we need this for a lot of routes, but it might be the +simplest solution right now. + ### Code in `spec/` When you're testing EE-only features, avoid adding examples to the @@ -375,6 +644,7 @@ information on managing page-specific javascript within EE. To separate EE-specific styles in SCSS files, if a component you're adding styles for is limited to only EE, it is better to have a separate SCSS file in appropriate directory within `app/assets/stylesheets`. +See [backporting changes](#backporting-changes) for instructions on how to merge changes safely. In some cases, this is not entirely possible or creating dedicated SCSS file is an overkill, e.g. a text style of some component is different for EE. In such cases, @@ -405,14 +675,28 @@ to avoid conflicts during CE to EE merge. } } -/* EE-specific styles */ +// EE-specific start .section-body.ee-section-body { .section-title { background: $gl-header-color-cyan; } } +// EE-specific end ``` +### Backporting changes from EE to CE + +When working in EE-specific features, you might have to tweak a few files that are not EE-specific. Here is a workflow to make sure those changes end up backported safely into CE too. +(This approach does not refer to changes introduced via [csslab](https://gitlab.com/gitlab-org/csslab/).) + +1. **Make your changes in the EE branch.** If possible, keep a separated commit (to be squashed) to help backporting and review. +1. **Open merge request to EE project.** +1. **Apply the changes you made to CE files in a branch of the CE project.** (Tip: Use `patch` with the diff from your commit in EE branch) +1. **Open merge request to CE project**, referring it's a backport of EE changes and link to MR open in EE. +1. Once EE MR is merged, the MR towards CE can be merged. **But not before**. + +**Note:** regarding SCSS, make sure the files living outside `/ee/` don't diverge between CE and EE projects. + ## gitlab-svgs Conflicts in `app/assets/images/icons.json` or `app/assets/images/icons.svg` can |