From aaf16997303a470809c8be442a537983b49f0f7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 5 Dec 2017 17:49:49 +0100 Subject: Move the "Limit conflicts with EE" doc to "Automatic CE-> EE merge" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- PROCESS.md | 6 +- doc/development/README.md | 2 +- doc/development/automatic_ce_ee_merge.md | 91 ++++++++ doc/development/ee_features.md | 6 + doc/development/limit_ee_conflicts.md | 347 ------------------------------- lib/gitlab/ee_compat_check.rb | 6 +- 6 files changed, 105 insertions(+), 353 deletions(-) create mode 100644 doc/development/automatic_ce_ee_merge.md delete mode 100644 doc/development/limit_ee_conflicts.md diff --git a/PROCESS.md b/PROCESS.md index 7c8db689256..758e773ae4d 100644 --- a/PROCESS.md +++ b/PROCESS.md @@ -130,7 +130,8 @@ freeze date (the 7th) should have a corresponding Enterprise Edition merge request, even if there are no conflicts. This is to reduce the size of the subsequent EE merge, as we often merge a lot to CE on the release date. For more information, see -[limit conflicts with EE when developing on CE][limit_ee_conflicts]. +[Automatic CE->EE merge][automatic_ce_ee_merge] and +[Guidelines for implementing Enterprise Edition feature][ee_features]. ### After the 7th @@ -281,4 +282,5 @@ still an issue I encourage you to open it on the [GitLab.com issue tracker](http ["Implement design & UI elements" guidelines]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#implement-design-ui-elements [Thoughtbot code review guide]: https://github.com/thoughtbot/guides/tree/master/code-review [done]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#definition-of-done -[limit_ee_conflicts]: https://docs.gitlab.com/ce/development/limit_ee_conflicts.html +[automatic_ce_ee_merge]: https://docs.gitlab.com/ce/development/automatic_ce_ee_merge.html +[ee_features]: https://docs.gitlab.com/ce/development/ee_features.html diff --git a/doc/development/README.md b/doc/development/README.md index 6892838be7f..944880d8ac7 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -16,7 +16,7 @@ comments: false - [GitLab core team & GitLab Inc. contribution process](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/PROCESS.md) - [Generate a changelog entry with `bin/changelog`](changelog.md) - [Code review guidelines](code_review.md) for reviewing code and having code reviewed. -- [Limit conflicts with EE when developing on CE](limit_ee_conflicts.md) +- [Automatic CE->EE merge](automatic_ce_ee_merge.md) ## UX and frontend guides diff --git a/doc/development/automatic_ce_ee_merge.md b/doc/development/automatic_ce_ee_merge.md new file mode 100644 index 00000000000..1a2a968ef54 --- /dev/null +++ b/doc/development/automatic_ce_ee_merge.md @@ -0,0 +1,91 @@ +# Automatic CE->EE merge + +GitLab Community Edition is merged automatically every 3 hours into the +Enterprise Edition (look for the [`CE Upstream` merge requests]). + +This merge is done automatically in a +[scheduled pipeline](https://gitlab.com/gitlab-org/release-tools/-/jobs/43201679). + +If a merge is already in progress, the job [doesn't create a new one](https://gitlab.com/gitlab-org/release-tools/-/jobs/43157687). + +**If you are pinged in a `CE Upstream` merge request to resolve a conflict, +please resolve the conflict as soon as possible or ask someone else to do it!** + +>**Note:** +It's ok to resolve more conflicts than the one that you are asked to resolve. In +that case, it's a good habit to ask for a double-check on your resolution by +someone who is familiar with the code you touched. + +### Always merge EE merge request before their CE counterpart + +**In order to avoid conflicts in the CE->EE merge, you should always merge the +EE version of your CE merge request (if present).** + +The rationale for this is that as CE->EE merges are done automatically every few +hours, it can happen that: + +1. A CE merge request that needs EE-specific changes is merged +1. The automatic CE->EE merge happens +1. Conflicts due to the CE merge request occur since its EE merge request isn't + merged yet +1. The automatic merge bot will ping someone to resolve the conflict **that are + already resolved in the EE merge request that isn't merged yet** +1. That's a waste of time, and that's why you should merge EE merge request + before their CE counterpart + +## Avoiding CE->EE merge conflicts beforehand + +To avoid the conflicts beforehand, check out the +[Guidelines for implementing Enterprise Edition feature](ee_features.md). + +In any case, the CI `ee_compat_check` job will tell you if you need to open an +EE version of your CE merge request. + +### Conflicts detection in CE merge requests + +For each commit (except on `master`), the `rake ee_compat_check` CI job tries to +detect if the current branch's changes will conflict during the CE->EE merge. + +The job reports what files are conflicting and how to setup a merge request +against EE. + +#### How the job works? + +1. Generates the diff between your branch and current CE `master` +1. Tries to apply it to current EE `master` +1. If it applies cleanly, the job succeeds, otherwise... +1. Detects a branch with the `ee-` prefix or `-ee` suffix in EE +1. If it exists, generate the diff between this branch and current EE `master` +1. Tries to apply it to current EE `master` +1. If it applies cleanly, the job succeeds + +In the case where the job fails, it means you should create a `ee-` +or `-ee` branch, push it to EE and open a merge request against EE +`master`. +At this point if you retry the failing job in your CE merge request, it should +now pass. + +Notes: + +- This task is not a silver-bullet, its current goal is to bring awareness to + developers that their work needs to be ported to EE. +- Community contributors shouldn't submit merge requests against EE, but + reviewers should take actions by either creating such EE merge request or + asking a GitLab developer to do it **before the merge request is merged**. +- If you branch is too far behind `master`, the job will fail. In that case you + should rebase your branch upon latest `master`. +- Code reviews for merge requests often consist of multiple iterations of + feedback and fixes. There is no need to update your EE MR after each + iteration. Instead, create an EE MR as soon as you see the + `ee_compat_check` job failing. After you receive the final approval + from a Maintainer (but **before the CE MR is merged**) update the EE MR. + This helps to identify significant conflicts sooner, but also reduces the + number of times you have to resolve conflicts. +- Please remember to + [always have you EE merge request merged before the CE one](#always-merge-ee-merge-request-before-their-ce-counterpart). +- You can use [`git rerere`](https://git-scm.com/blog/2010/03/08/rerere.html) + to avoid resolving the same conflicts multiple times. + +--- + +[Return to Development documentation](README.md) diff --git a/doc/development/ee_features.md b/doc/development/ee_features.md index 932a44f65e4..eb33d17443a 100644 --- a/doc/development/ee_features.md +++ b/doc/development/ee_features.md @@ -380,3 +380,9 @@ to avoid conflicts during CE to EE merge. } } ``` + +## gitlab-svgs + +Conflicts in `app/assets/images/icons.json` or `app/assets/images/icons.svg` can +be resolved simply by regenerating those assets with +[`yarn run svg`](https://gitlab.com/gitlab-org/gitlab-svgs). diff --git a/doc/development/limit_ee_conflicts.md b/doc/development/limit_ee_conflicts.md deleted file mode 100644 index ba82babb38a..00000000000 --- a/doc/development/limit_ee_conflicts.md +++ /dev/null @@ -1,347 +0,0 @@ -# Limit conflicts with EE when developing on CE - -This guide contains best-practices for avoiding conflicts between CE and EE. - -## Daily CE Upstream merge - -GitLab Community Edition is merged daily into the Enterprise Edition (look for -the [`CE Upstream` merge requests]). The daily merge is currently done manually -by four individuals. - -**If a developer pings you in a `CE Upstream` merge request for help with -resolving conflicts, please help them because it means that you didn't do your -job to reduce the conflicts nor to ease their resolution in the first place!** - -To avoid the conflicts beforehand when working on CE, there are a few tools and -techniques that can help you: - -- know what are the usual types of conflicts and how to prevent them -- the CI `rake ee_compat_check` job tells you if you need to open an EE-version - of your CE merge request - -[`CE Upstream` merge requests]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests?label_name%5B%5D=CE+upstream - -## Check the status of the CI `rake ee_compat_check` job - -For each commit (except on `master`), the `rake ee_compat_check` CI job tries to -detect if the current branch's changes will conflict during the CE->EE merge. - -The job reports what files are conflicting and how to setup a merge request -against EE. Here is roughly how it works: - -1. Generates the diff between your branch and current CE `master` -1. Tries to apply it to current EE `master` -1. If it applies cleanly, the job succeeds, otherwise... -1. Detects a branch with the `-ee` suffix in EE -1. If it exists, generate the diff between this branch and current EE `master` -1. Tries to apply it to current EE `master` -1. If it applies cleanly, the job succeeds - -In the case where the job fails, it means you should create a `-ee` -branch, push it to EE and open a merge request against EE `master`. At this -point if you retry the failing job in your CE merge request, it should now pass. - -Notes: - -- This task is not a silver-bullet, its current goal is to bring awareness to - developers that their work needs to be ported to EE. -- Community contributors shouldn't submit merge requests against EE, but - reviewers should take actions by either creating such EE merge request or - asking a GitLab developer to do it once the merge request is merged. -- If you branch is more than 500 commits behind `master`, the job will fail and - you should rebase your branch upon latest `master`. -- Code reviews for merge requests often consist of multiple iterations of - feedback and fixes. There is no need to update your EE MR after each - iteration. Instead, create an EE MR as soon as you see the - `rake ee_compat_check` job failing. After you receive the final acceptance - from a Maintainer (but before the CE MR is merged) update the EE MR. - This helps to identify significant conflicts sooner, but also reduces the - number of times you have to resolve conflicts. -- You can use [`git rerere`](https://git-scm.com/blog/2010/03/08/rerere.html) - to avoid resolving the same conflicts multiple times. - -## Possible type of conflicts - -### Controllers - -#### List or arrays are augmented in EE - -In controllers, the most common type of conflict is with `before_action` that -has a list of actions in CE but EE adds some actions to that list. - -The same problem often occurs for `params.require` / `params.permit` calls. - -##### Mitigations - -Separate CE and EE actions/keywords. For instance for `params.require` in -`ProjectsController`: - -```ruby -def project_params - params.require(:project).permit(project_params_ce) - # On EE, this is always: - # params.require(:project).permit(project_params_ce << project_params_ee) -end - -# Always returns an array of symbols, created however best fits the use case. -# It _should_ be sorted alphabetically. -def project_params_ce - %i[ - description - name - path - ] -end - -# (On EE) -def project_params_ee - %i[ - approvals_before_merge - approver_group_ids - approver_ids - ... - ] -end -``` - -#### Additional condition(s) in EE - -For instance for LDAP: - -```diff - def destroy - @key = current_user.keys.find(params[:id]) - - @key.destroy - + @key.destroy unless @key.is_a? LDAPKey - - respond_to do |format| -``` - -Or for Geo: - -```diff -def after_sign_out_path_for(resource) -- current_application_settings.after_sign_out_path.presence || new_user_session_path -+ if Gitlab::Geo.secondary? -+ Gitlab::Geo.primary_node.oauth_logout_url(@geo_logout_state) -+ else -+ current_application_settings.after_sign_out_path.presence || new_user_session_path -+ end -end -``` - -Or even for audit log: - -```diff -def approve_access_request -- Members::ApproveAccessRequestService.new(membershipable, current_user, params).execute -+ member = Members::ApproveAccessRequestService.new(membershipable, current_user, params).execute -+ -+ log_audit_event(member, action: :create) - - redirect_to polymorphic_url([membershipable, :members]) -end -``` - -### Views - -#### Additional view code in EE - -A block of code added in CE conflicts because there is already another block -at the same place in EE - -##### Mitigations - -Blocks of code that are EE-specific should be moved to partials as much as -possible to avoid conflicts with big chunks of HAML code that that are not fun -to resolve when you add the indentation to the equation. - -For instance this kind of thing: - -```haml -.form-group.detail-page-description - = form.label :description, 'Description', class: 'control-label' - .col-sm-10 - = render layout: 'projects/md_preview', locals: { preview_class: "md-preview", referenced_users: true } do - = render 'projects/zen', f: form, attr: :description, - classes: 'note-textarea', - placeholder: "Write a comment or drag your files here...", - supports_quick_actions: !issuable.persisted? - = render 'projects/notes/hints', supports_quick_actions: !issuable.persisted? - .clearfix - .error-alert -- if issuable.is_a?(Issue) - .form-group - .col-sm-offset-2.col-sm-10 - .checkbox - = form.label :confidential do - = form.check_box :confidential - This issue is confidential and should only be visible to team members with at least Reporter access. -- if can?(current_user, :"admin_#{issuable.to_ability_name}", issuable.project) - - has_due_date = issuable.has_attribute?(:due_date) - %hr - .row - %div{ class: (has_due_date ? "col-lg-6" : "col-sm-12") } - .form-group.issue-assignee - = form.label :assignee_id, "Assignee", class: "control-label #{"col-lg-4" if has_due_date}" - .col-sm-10{ class: ("col-lg-8" if has_due_date) } - .issuable-form-select-holder - - if issuable.assignee_id - = form.hidden_field :assignee_id - = dropdown_tag(user_dropdown_label(issuable.assignee_id, "Assignee"), options: { toggle_class: "js-dropdown-keep-input js-user-search js-issuable-form-dropdown js-assignee-search", title: "Select assignee", filter: true, dropdown_class: "dropdown-menu-user dropdown-menu-selectable dropdown-menu-assignee js-filter-submit", - placeholder: "Search assignee", data: { first_user: current_user.try(:username), null_user: true, current_user: true, project_id: project.try(:id), selected: issuable.assignee_id, field_name: "#{issuable.class.model_name.param_key}[assignee_id]", default_label: "Assignee"} }) - .form-group.issue-milestone - = form.label :milestone_id, "Milestone", class: "control-label #{"col-lg-4" if has_due_date}" - .col-sm-10{ class: ("col-lg-8" if has_due_date) } - .issuable-form-select-holder - = render "shared/issuable/milestone_dropdown", selected: issuable.milestone, name: "#{issuable.class.model_name.param_key}[milestone_id]", show_any: false, show_upcoming: false, extra_class: "js-issuable-form-dropdown js-dropdown-keep-input", dropdown_title: "Select milestone" - .form-group - - has_labels = @labels && @labels.any? - = form.label :label_ids, "Labels", class: "control-label #{"col-lg-4" if has_due_date}" - = form.hidden_field :label_ids, multiple: true, value: '' - .col-sm-10{ class: "#{"col-lg-8" if has_due_date} #{'issuable-form-padding-top' if !has_labels}" } - .issuable-form-select-holder - = render "shared/issuable/label_dropdown", classes: ["js-issuable-form-dropdown"], selected: issuable.labels, data_options: { field_name: "#{issuable.class.model_name.param_key}[label_ids][]", show_any: false }, dropdown_title: "Select label" - - if issuable.respond_to?(:weight) - - weight_options = Issue.weight_options - - weight_options.delete(Issue::WEIGHT_ALL) - - weight_options.delete(Issue::WEIGHT_ANY) - .form-group - = form.label :label_ids, class: "control-label #{"col-lg-4" if has_due_date}" do - Weight - .col-sm-10{ class: ("col-lg-8" if has_due_date) } - .issuable-form-select-holder - - if issuable.weight - = form.hidden_field :weight - = dropdown_tag(issuable.weight || "Weight", options: { title: "Select weight", toggle_class: 'js-weight-select js-issuable-form-weight', dropdown_class: "dropdown-menu-selectable dropdown-menu-weight", - placeholder: "Search weight", data: { field_name: "#{issuable.class.model_name.param_key}[weight]" , default_label: "Weight" } }) do - %ul - - weight_options.each do |weight| - %li - %a{href: "#", data: { id: weight, none: weight === Issue::WEIGHT_NONE }, class: ("is-active" if issuable.weight == weight)} - = weight - - if has_due_date - .col-lg-6 - .form-group - = form.label :due_date, "Due date", class: "control-label" - .col-sm-10 - .issuable-form-select-holder - = form.text_field :due_date, id: "issuable-due-date", class: "datepicker form-control", placeholder: "Select due date" -``` - -could be simplified by using partials: - -```haml -= render 'shared/issuable/form/description', issuable: issuable, form: form - -- if issuable.respond_to?(:confidential) - .form-group - .col-sm-offset-2.col-sm-10 - .checkbox - = form.label :confidential do - = form.check_box :confidential - This issue is confidential and should only be visible to team members with at least Reporter access. - -= render 'shared/issuable/form/metadata', issuable: issuable, form: form -``` - -and then the `app/views/shared/issuable/form/_metadata.html.haml` could be as follows: - -```haml -- issuable = local_assigns.fetch(:issuable) - -- return unless can?(current_user, :"admin_#{issuable.to_ability_name}", issuable.project) - -- has_due_date = issuable.has_attribute?(:due_date) -- has_labels = @labels && @labels.any? -- form = local_assigns.fetch(:form) - -%hr -.row - %div{ class: (has_due_date ? "col-lg-6" : "col-sm-12") } - .form-group.issue-assignee - = form.label :assignee_id, "Assignee", class: "control-label #{"col-lg-4" if has_due_date}" - .col-sm-10{ class: ("col-lg-8" if has_due_date) } - .issuable-form-select-holder - - if issuable.assignee_id - = form.hidden_field :assignee_id - = dropdown_tag(user_dropdown_label(issuable.assignee_id, "Assignee"), options: { toggle_class: "js-dropdown-keep-input js-user-search js-issuable-form-dropdown js-assignee-search", title: "Select assignee", filter: true, dropdown_class: "dropdown-menu-user dropdown-menu-selectable dropdown-menu-assignee js-filter-submit", - placeholder: "Search assignee", data: { first_user: current_user.try(:username), null_user: true, current_user: true, project_id: issuable.project.try(:id), selected: issuable.assignee_id, field_name: "#{issuable.class.model_name.param_key}[assignee_id]", default_label: "Assignee"} }) - .form-group.issue-milestone - = form.label :milestone_id, "Milestone", class: "control-label #{"col-lg-4" if has_due_date}" - .col-sm-10{ class: ("col-lg-8" if has_due_date) } - .issuable-form-select-holder - = render "shared/issuable/milestone_dropdown", selected: issuable.milestone, name: "#{issuable.class.model_name.param_key}[milestone_id]", show_any: false, show_upcoming: false, extra_class: "js-issuable-form-dropdown js-dropdown-keep-input", dropdown_title: "Select milestone" - .form-group - - has_labels = @labels && @labels.any? - = form.label :label_ids, "Labels", class: "control-label #{"col-lg-4" if has_due_date}" - = form.hidden_field :label_ids, multiple: true, value: '' - .col-sm-10{ class: "#{"col-lg-8" if has_due_date} #{'issuable-form-padding-top' if !has_labels}" } - .issuable-form-select-holder - = render "shared/issuable/label_dropdown", classes: ["js-issuable-form-dropdown"], selected: issuable.labels, data_options: { field_name: "#{issuable.class.model_name.param_key}[label_ids][]", show_any: false }, dropdown_title: "Select label" - - = render "shared/issuable/form/weight", issuable: issuable, form: form - - - if has_due_date - .col-lg-6 - .form-group - = form.label :due_date, "Due date", class: "control-label" - .col-sm-10 - .issuable-form-select-holder - = form.text_field :due_date, id: "issuable-due-date", class: "datepicker form-control", placeholder: "Select due date" -``` - -and then the `app/views/shared/issuable/form/_weight.html.haml` could be as follows: - -```haml -- issuable = local_assigns.fetch(:issuable) - -- return unless issuable.respond_to?(:weight) - -- has_due_date = issuable.has_attribute?(:due_date) -- form = local_assigns.fetch(:form) - -.form-group - = form.label :label_ids, class: "control-label #{"col-lg-4" if has_due_date}" do - Weight - .col-sm-10{ class: ("col-lg-8" if has_due_date) } - .issuable-form-select-holder - - if issuable.weight - = form.hidden_field :weight - - = weight_dropdown_tag(issuable, toggle_class: 'js-issuable-form-weight') do - %ul - - Issue.weight_options.each do |weight| - %li - %a{ href: '#', data: { id: weight, none: weight === Issue::WEIGHT_NONE }, class: ("is-active" if issuable.weight == weight) } - = weight -``` - -Note: - -- The safeguards at the top allow to get rid of an unneccessary indentation level -- Here we only moved the 'Weight' code to a partial since this is the only - EE-specific code in that view, so it's the most likely to conflict, but you - are encouraged to use partials even for code that's in CE to logically split - big views into several smaller files. - -#### Indentation issue - -Sometimes a code block is indented more or less in EE because there's an -additional condition. - -##### Mitigations - -Blocks of code that are EE-specific should be moved to partials as much as -possible to avoid conflicts with big chunks of HAML code that that are not fun -to resolve when you add the indentation in the equation. - -### Assets - -#### gitlab-svgs - -Conflicts in `app/assets/images/icons.json` or `app/assets/images/icons.svg` can be resolved simply by regenerating those assets with [`yarn run svg`](https://gitlab.com/gitlab-org/gitlab-svgs). - ---- - -[Return to Development documentation](README.md) diff --git a/lib/gitlab/ee_compat_check.rb b/lib/gitlab/ee_compat_check.rb index 4a9d3e52fae..37face8e7d0 100644 --- a/lib/gitlab/ee_compat_check.rb +++ b/lib/gitlab/ee_compat_check.rb @@ -280,7 +280,7 @@ module Gitlab The `#{branch}` branch applies cleanly to EE/master! Much ❤️! For more information, see - https://docs.gitlab.com/ce/development/limit_ee_conflicts.html#check-the-rake-ee_compat_check-in-your-merge-requests + https://docs.gitlab.com/ce/development/automatic_ce_ee_merge.html #{THANKS_FOR_READING_BANNER} } end @@ -357,7 +357,7 @@ module Gitlab Once this is done, you can retry this failed build, and it should pass. Stay 💪 ! For more information, see - https://docs.gitlab.com/ce/development/limit_ee_conflicts.html#check-the-rake-ee_compat_check-in-your-merge-requests + https://docs.gitlab.com/ce/development/automatic_ce_ee_merge.html #{THANKS_FOR_READING_BANNER} } end @@ -378,7 +378,7 @@ module Gitlab retry this build. Stay 💪 ! For more information, see - https://docs.gitlab.com/ce/development/limit_ee_conflicts.html#check-the-rake-ee_compat_check-in-your-merge-requests + https://docs.gitlab.com/ce/development/automatic_ce_ee_merge.html #{THANKS_FOR_READING_BANNER} } end -- cgit v1.2.1 From dd0223f530a0616ca6c0e40db3860f61322b6553 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 6 Dec 2017 16:30:03 +0100 Subject: Address Robert's and Axil's feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- PROCESS.md | 2 +- doc/development/automatic_ce_ee_merge.md | 26 ++++++++++++++------------ doc/development/ee_features.md | 2 +- doc/development/writing_documentation.md | 2 +- 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/PROCESS.md b/PROCESS.md index 758e773ae4d..3fcf676b302 100644 --- a/PROCESS.md +++ b/PROCESS.md @@ -131,7 +131,7 @@ request, even if there are no conflicts. This is to reduce the size of the subsequent EE merge, as we often merge a lot to CE on the release date. For more information, see [Automatic CE->EE merge][automatic_ce_ee_merge] and -[Guidelines for implementing Enterprise Edition feature][ee_features]. +[Guidelines for implementing Enterprise Edition features][ee_features]. ### After the 7th diff --git a/doc/development/automatic_ce_ee_merge.md b/doc/development/automatic_ce_ee_merge.md index 1a2a968ef54..9e59ddc8cce 100644 --- a/doc/development/automatic_ce_ee_merge.md +++ b/doc/development/automatic_ce_ee_merge.md @@ -5,7 +5,6 @@ Enterprise Edition (look for the [`CE Upstream` merge requests]). This merge is done automatically in a [scheduled pipeline](https://gitlab.com/gitlab-org/release-tools/-/jobs/43201679). - If a merge is already in progress, the job [doesn't create a new one](https://gitlab.com/gitlab-org/release-tools/-/jobs/43157687). **If you are pinged in a `CE Upstream` merge request to resolve a conflict, @@ -16,10 +15,12 @@ It's ok to resolve more conflicts than the one that you are asked to resolve. In that case, it's a good habit to ask for a double-check on your resolution by someone who is familiar with the code you touched. -### Always merge EE merge request before their CE counterpart +[`CE Upstream` merge requests]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests?label_name%5B%5D=CE+upstream + +### Always merge EE merge requests before their CE counterparts **In order to avoid conflicts in the CE->EE merge, you should always merge the -EE version of your CE merge request (if present).** +EE version of your CE merge request first, if present.** The rationale for this is that as CE->EE merges are done automatically every few hours, it can happen that: @@ -30,26 +31,27 @@ hours, it can happen that: merged yet 1. The automatic merge bot will ping someone to resolve the conflict **that are already resolved in the EE merge request that isn't merged yet** -1. That's a waste of time, and that's why you should merge EE merge request - before their CE counterpart + +That's a waste of time, and that's why you should merge EE merge request before +their CE counterpart. ## Avoiding CE->EE merge conflicts beforehand To avoid the conflicts beforehand, check out the -[Guidelines for implementing Enterprise Edition feature](ee_features.md). +[Guidelines for implementing Enterprise Edition features](ee_features.md). In any case, the CI `ee_compat_check` job will tell you if you need to open an EE version of your CE merge request. ### Conflicts detection in CE merge requests -For each commit (except on `master`), the `rake ee_compat_check` CI job tries to +For each commit (except on `master`), the `ee_compat_check` CI job tries to detect if the current branch's changes will conflict during the CE->EE merge. The job reports what files are conflicting and how to setup a merge request against EE. -#### How the job works? +#### How the job works 1. Generates the diff between your branch and current CE `master` 1. Tries to apply it to current EE `master` @@ -69,9 +71,9 @@ Notes: - This task is not a silver-bullet, its current goal is to bring awareness to developers that their work needs to be ported to EE. -- Community contributors shouldn't submit merge requests against EE, but - reviewers should take actions by either creating such EE merge request or - asking a GitLab developer to do it **before the merge request is merged**. +- Community contributors shouldn't be required to submit merge requests against + EE, but reviewers should take actions by either creating such EE merge request + or asking a GitLab developer to do it **before the merge request is merged**. - If you branch is too far behind `master`, the job will fail. In that case you should rebase your branch upon latest `master`. - Code reviews for merge requests often consist of multiple iterations of @@ -82,7 +84,7 @@ Notes: This helps to identify significant conflicts sooner, but also reduces the number of times you have to resolve conflicts. - Please remember to - [always have you EE merge request merged before the CE one](#always-merge-ee-merge-request-before-their-ce-counterpart). + [always have your EE merge request merged before the CE version](#always-merge-ee-merge-requests-before-their-ce-counterparts). - You can use [`git rerere`](https://git-scm.com/blog/2010/03/08/rerere.html) to avoid resolving the same conflicts multiple times. diff --git a/doc/development/ee_features.md b/doc/development/ee_features.md index eb33d17443a..1af839a27e1 100644 --- a/doc/development/ee_features.md +++ b/doc/development/ee_features.md @@ -1,4 +1,4 @@ -# Guidelines for implementing Enterprise Edition feature +# Guidelines for implementing Enterprise Edition features - **Write the code and the tests.**: As with any code, EE features should have good test coverage to prevent regressions. diff --git a/doc/development/writing_documentation.md b/doc/development/writing_documentation.md index b6def7ef541..48e04a40050 100644 --- a/doc/development/writing_documentation.md +++ b/doc/development/writing_documentation.md @@ -142,7 +142,7 @@ tests. If it doesn't, the whole test suite will run (including docs). --- When you submit a merge request to GitLab Community Edition (CE), there is an -additional job called `rake ee_compat_check` that runs against Enterprise +additional job called `ee_compat_check` that runs against Enterprise Edition (EE) and checks if your changes can apply cleanly to the EE codebase. If that job fails, read the instructions in the job log for what to do next. Contributors do not need to submit their changes to EE, GitLab Inc. employees -- cgit v1.2.1 From cd03bf847d0edff179317818e95efb4551dabecf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 6 Dec 2017 17:23:53 +0100 Subject: Add "Guidelines for implementing Enterprise Edition features" in CE development doc since the doc page is already there MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- doc/development/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/development/README.md b/doc/development/README.md index 944880d8ac7..7e4c767692a 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -17,6 +17,7 @@ comments: false - [Generate a changelog entry with `bin/changelog`](changelog.md) - [Code review guidelines](code_review.md) for reviewing code and having code reviewed. - [Automatic CE->EE merge](automatic_ce_ee_merge.md) +- [Guidelines for implementing Enterprise Edition features](ee_features.md) ## UX and frontend guides -- cgit v1.2.1