diff options
| -rw-r--r-- | PROCESS.md | 6 | ||||
| -rw-r--r-- | doc/development/README.md | 2 | ||||
| -rw-r--r-- | doc/development/automatic_ce_ee_merge.md | 91 | ||||
| -rw-r--r-- | doc/development/ee_features.md | 6 | ||||
| -rw-r--r-- | doc/development/limit_ee_conflicts.md | 347 | ||||
| -rw-r--r-- | lib/gitlab/ee_compat_check.rb | 6 |
6 files changed, 105 insertions, 353 deletions
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-<ce_branch>` +or `<ce_branch>-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 `<ce_branch>-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 |