Merge branch 'master' of gitlab.com:gitlab-org/gitlab-ee into ce-to-ee-2017-08-25

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

app/assets/javascripts/geo_nodes.js

@@ -13,6 +13,7 @@ class GeoNodeStatus {
     this.$el = $(el);
     this.$icon = $('.js-geo-node-icon', this.$el);
     this.$loadingIcon = $('.js-geo-node-loading', this.$el);
+    this.$dbReplicationLag = $('.js-db-replication-lag', this.$status);
     this.$healthStatus = $('.js-health-status', this.$el);
     this.$status = $('.js-geo-node-status', this.$el);
     this.$repositoriesSynced = $('.js-repositories-synced', this.$status);
@@ -36,6 +37,15 @@ class GeoNodeStatus {
     $.getJSON(this.endpoint, (status) => {
       this.setStatusIcon(status.healthy);
       this.setHealthStatus(status.healthy);
+
+      // Replication lag can be nil if the secondary isn't actually streaming
+      if (status.db_replication_lag) {
+        const parsedTime = gl.utils.prettyTime.parseSeconds(status.db_replication_lag);
+        this.$dbReplicationLag.text(gl.utils.prettyTime.stringifyTime(parsedTime));
+      } else {
+        this.$dbReplicationLag.text('UNKNOWN');
+      }
+
       this.$repositoriesSynced.html(`${status.repositories_synced_count}/${status.repositories_count} (${status.repositories_synced_in_percentage})`);
       this.$repositoriesFailed.html(status.repositories_failed_count);
       this.$lfsObjectsSynced.html(`${status.lfs_objects_synced_count}/${status.lfs_objects_count} (${status.lfs_objects_synced_in_percentage})`);

app/models/geo_node_status.rb

@@ -14,6 +14,14 @@ class GeoNodeStatus
     health.blank?
   end
 
+  def db_replication_lag
+    @db_replication_lag ||= Gitlab::Geo::HealthCheck.db_replication_lag
+  end
+
+  def db_replication_lag=(value)
+    @db_replication_lag = value
+  end
+
   def repositories_count
     @repositories_count ||= repositories.count
   end

app/models/merge_request.rb

@@ -100,7 +100,7 @@ class MergeRequest < ActiveRecord::Base
   validates :merge_user, presence: true, if: :merge_when_pipeline_succeeds?, unless: :importing?
   validate :validate_branches, unless: [:allow_broken, :importing?, :closed_without_fork?]
   validate :validate_fork, unless: :closed_without_fork?
-  validate :validate_approvals_before_merge
+  validate :validate_approvals_before_merge, unless: :importing?
   validate :validate_target_project, on: :create
 
   scope :by_source_or_target_branch, ->(branch_name) do

app/serializers/geo_node_status_entity.rb

@@ -14,6 +14,8 @@ class GeoNodeStatusEntity < Grape::Entity
     number_to_percentage(node.attachments_synced_in_percentage, precision: 2)
   end
 
+  expose :db_replication_lag
+
   expose :lfs_objects_count
   expose :lfs_objects_synced_count
   expose :lfs_objects_synced_in_percentage do |node|

app/services/geo/node_status_service.rb

@@ -5,6 +5,7 @@ module Geo
 
     KEYS = %w(
       health
+      db_replication_lag
       repositories_count
       repositories_synced_count
       repositories_failed_count

app/services/groups/nested_create_service.rb

@@ -15,6 +15,10 @@ module Groups
         return group
       end
 
+      if group_path.include?('/') && !Group.supports_nested_groups?
+        raise 'Nested groups are not supported on MySQL'
+      end
+
       create_group_path
     end
 

app/views/admin/geo_nodes/index.html.haml

@@ -45,6 +45,10 @@
                     %span.help-block
                       Health Status:
                       %span.js-health-status
+                %p
+                  %span.help-block
+                    Database replication lag:
+                    %strong.node-info.js-db-replication-lag
                 %p
                   %span.help-block
                     Repositories synced:

app/views/layouts/nav/_new_admin_sidebar.html.haml

@@ -75,7 +75,6 @@
             = link_to admin_requests_profiles_path, title: 'Requests Profiles' do
               %span
                 Requests Profiles
-
           = render 'layouts/nav/ee/new_admin_monitoring_sidebar'
 
       = nav_link(controller: :broadcast_messages) do
@@ -84,6 +83,7 @@
             = custom_icon('messages')
           %span.nav-item-name
             Messages
+
       = nav_link(controller: [:hooks, :hook_logs]) do
         = link_to admin_hooks_path, title: 'Hooks' do
           .nav-icon-container

changelogs/unreleased-ee/fix-approvals_before_merge-import.yml

+---
+title: Fix approvals before merge error while importing projects
+merge_request:
+author:
+type: fixed

changelogs/unreleased-ee/zj-improve-object-store-rake-task.yml

+---
+title: Use a logger for the artifacts migration rake task
+merge_request:
+author:
+type: changed

doc/development/doc_styleguide.md

@@ -113,13 +113,12 @@ merge request.
 
 ## Links
 
-- If a link makes the paragraph to span across multiple lines, do not use
-  the regular Markdown approach: `[Text](https://example.com)`. Instead use
-  `[Text][identifier]` and at the very bottom of the document add:
-  `[identifier]: https://example.com`. This is another way to create Markdown
-  links which keeps the document clear and concise. Bonus points if you also
-  add an alternative text: `[identifier]: https://example.com "Alternative text"`
-  that appears when hovering your mouse on a link
+- Use the regular inline link markdown markup `[Text](https://example.com)`.
+  It's easier to read, review, and maintain.
+- If there's a link that repeats several times through the same document,
+  you can use `[Text][identifier]` and at the bottom of the section or the
+  document add: `[identifier]: https://example.com`, in which case, we do
+  encourage you to also add an alternative text: `[identifier]: https://example.com "Alternative text"` that appears when hovering your mouse on a link.
 
 ### Linking to inline docs
 

doc/gitlab-geo/configuration.md

@@ -207,6 +207,9 @@ You can monitor the status of the syncing process on a secondary node
 by visiting the primary node's **Admin Area ➔ Geo Nodes** (`/admin/geo_nodes`)
 in your browser.
 
+Please note that if `git_data_dirs` is customized on the primary for multiple
+repository shards you must duplicate the same configuration on the secondary.
+
 ![GitLab Geo dashboard](img/geo-node-dashboard.png)
 
 Disabling a secondary node stops the syncing process.

doc/user/index.md

@@ -119,6 +119,13 @@ When performing inline reviews to implementations
 to your codebase through merge requests you can
 gather feedback through [resolvable discussions](discussions/index.md#resolvable-discussions).
 
+### GitLab Flavored Markdown (GFM)
+
+Read through the [GFM documentation](markdown.md) to learn how to apply
+the best of GitLab Flavored Markdown in your discussions, comments,
+issues and merge requests descriptions, and everywhere else GMF is
+supported.
+
 ## Todos
 
 Never forget to reply to your collaborators. [GitLab Todos](../workflow/todos.md)

doc/user/project/integrations/project_services.md

@@ -38,6 +38,7 @@ Click on the service links to see further configuration instructions and details
 | [HipChat](hipchat.md) | Private group chat and IM |
 | [Irker (IRC gateway)](irker.md) | Send IRC messages, on update, to a list of recipients through an Irker gateway |
 | [JIRA](jira.md) | JIRA issue tracker |
+| [Jenkins](../../../integration/jenkins.md) | An extendable open source continuous integration server |
 | JetBrains TeamCity CI | A continuous integration and build server |
 | [Kubernetes](kubernetes.md) | A containerized deployment service |
 | [Mattermost slash commands](mattermost_slash_commands.md) | Mattermost chat and ChatOps slash commands |
@@ -46,7 +47,7 @@ Click on the service links to see further configuration instructions and details
 | Pipelines emails | Email the pipeline status to a list of recipients |
 | [Slack Notifications](slack.md) | Send GitLab events (e.g. issue created) to Slack as notifications |
 | [Slack slash commands](slack_slash_commands.md) | Use slash commands in Slack to control GitLab |
-| [GitLab Slack application](gitlab_slack_application.md) | Use Slack's official application 
+| [GitLab Slack application](gitlab_slack_application.md) | Use Slack's official application
 | PivotalTracker | Project Management Software (Source Commits Endpoint) |
 | [Prometheus](prometheus.md) | Monitor the performance of your deployed apps |
 | Pushover | Pushover makes it easy to get real-time notifications on your Android device, iPhone, iPad, and Desktop |

lib/api/entities.rb

@@ -947,6 +947,7 @@ module API
 
     class GeoNodeStatus < Grape::Entity
       expose :id
+      expose :db_replication_lag
       expose :health
       expose :healthy?, as: :healthy
       expose :repositories_count

lib/gitlab/geo/health_check.rb

@@ -7,6 +7,7 @@ module Gitlab
         return '' unless Gitlab::Geo.secondary?
         return 'The Geo database configuration file is missing.' unless Gitlab::Geo.geo_database_configured?
         return 'The Geo node has a database that is not configured for streaming replication with the primary node.' unless self.database_secondary?
+        return 'The Geo node does not appear to be replicating data from the primary node.' unless self.db_replication_lag.present?
 
         database_version  = self.get_database_version.to_i
         migration_version = self.get_migration_version.to_i
@@ -54,12 +55,24 @@ module Gitlab
       end
 
       def self.database_secondary?
-        raise NotImplementedError unless Gitlab::Database.postgresql?
-
         ActiveRecord::Base.connection.execute('SELECT pg_is_in_recovery()')
           .first
           .fetch('pg_is_in_recovery') == 't'
       end
+
+      def self.db_replication_lag
+        # Obtain the replication lag in seconds
+        ActiveRecord::Base.connection.execute('
+          SELECT CASE
+                 WHEN pg_last_xlog_receive_location() = pg_last_xlog_replay_location()
+                  THEN 0
+                 ELSE
+                  EXTRACT (EPOCH FROM now() - pg_last_xact_replay_timestamp())::INTEGER
+                 END
+                 AS replication_lag')
+          .first
+          .fetch('replication_lag')
+      end
     end
   end
 end

lib/tasks/gitlab/artifacts.rake

+require 'logger'
+
 desc "GitLab | Migrate files for artifacts to comply with new storage format"
 namespace :gitlab do
   namespace :artifacts do
     task migrate: :environment do
-      puts 'Artifacts'.color(:yellow)
+      logger = Logger.new(STDOUT)
+      logger.info('Starting transfer of artifacts')
+
       Ci::Build.joins(:project)
         .with_artifacts
-        .order(id: :asc)
         .where(artifacts_file_store: [nil, ArtifactUploader::LOCAL_STORE])
         .find_each(batch_size: 10) do |build|
         begin
-          print "Migrating job #{build.id} of size #{build.artifacts_size.to_i.bytes} to remote storage... "
           build.artifacts_file.migrate!(ArtifactUploader::REMOTE_STORE)
           build.artifacts_metadata.migrate!(ArtifactUploader::REMOTE_STORE)
-          puts "OK".color(:green)
+
+          logger.info("Transferred artifacts of #{build.id} of #{build.artifacts_size} to object storage")
         rescue => e
-          puts "Failed: #{e.message}".color(:red)
+          logger.error("Failed to transfer artifacts of #{build.id} with error: #{e.message}")
         end
       end
     end

spec/fixtures/api/schemas/geo_node_status.json

@@ -8,6 +8,7 @@
     "attachments_synced_count",
     "lfs_objects_count",
     "lfs_objects_synced_count",
+    "db_replication_lag",
     "repositories_count",
     "repositories_failed_count",
     "repositories_synced_count"
@@ -19,6 +20,7 @@
     "attachments_count": { "type": "integer" },
     "attachments_synced_count": { "type": "integer" },
     "attachments_synced_in_percentage": { "type": "string" },
+    "db_replication_lag": { "type": ["integer", "null"] },
     "lfs_objects_count": { "type": "integer" },
     "lfs_objects_synced_count": { "type": "integer" },
     "lfs_objects_synced_in_percentage": { "type": "string" },

spec/lib/gitlab/bare_repository_importer_spec.rb

@@ -2,67 +2,99 @@ require 'spec_helper'
 
 describe Gitlab::BareRepositoryImporter, repository: true do
   subject(:importer) { described_class.new('default', project_path) }
-  let(:project_path) { 'a-group/a-sub-group/a-project' }
+
   let!(:admin) { create(:admin) }
 
   before do
     allow(described_class).to receive(:log)
   end
 
-  describe '.execute' do
-    it 'creates a project for a repository in storage' do
-      FileUtils.mkdir_p(File.join(TestEnv.repos_path, "#{project_path}.git"))
-      fake_importer = double
+  shared_examples 'importing a repository' do
+    describe '.execute' do
+      it 'creates a project for a repository in storage' do
+        FileUtils.mkdir_p(File.join(TestEnv.repos_path, "#{project_path}.git"))
+        fake_importer = double
 
-      expect(described_class).to receive(:new).with('default', project_path)
-                                   .and_return(fake_importer)
-      expect(fake_importer).to receive(:create_project_if_needed)
+        expect(described_class).to receive(:new).with('default', project_path)
+                                     .and_return(fake_importer)
+        expect(fake_importer).to receive(:create_project_if_needed)
 
-      described_class.execute
-    end
+        described_class.execute
+      end
 
-    it 'skips wiki repos' do
-      FileUtils.mkdir_p(File.join(TestEnv.repos_path, 'the-group', 'the-project.wiki.git'))
+      it 'skips wiki repos' do
+        FileUtils.mkdir_p(File.join(TestEnv.repos_path, 'the-group', 'the-project.wiki.git'))
 
-      expect(described_class).to receive(:log).with(' * Skipping wiki repo')
-      expect(described_class).not_to receive(:new)
+        expect(described_class).to receive(:log).with(' * Skipping wiki repo')
+        expect(described_class).not_to receive(:new)
 
-      described_class.execute
+        described_class.execute
+      end
     end
-  end
 
-  describe '#initialize' do
-    context 'without admin users' do
-      let(:admin) { nil }
+    describe '#initialize' do
+      context 'without admin users' do
+        let(:admin) { nil }
 
-      it 'raises an error' do
-        expect { importer }.to raise_error(Gitlab::BareRepositoryImporter::NoAdminError)
+        it 'raises an error' do
+          expect { importer }.to raise_error(Gitlab::BareRepositoryImporter::NoAdminError)
+        end
       end
     end
-  end
 
-  describe '#create_project_if_needed' do
-    it 'starts an import for a project that did not exist' do
-      expect(importer).to receive(:create_project)
+    describe '#create_project_if_needed' do
+      it 'starts an import for a project that did not exist' do
+        expect(importer).to receive(:create_project)
+
+        importer.create_project_if_needed
+      end
+
+      it 'skips importing when the project already exists' do
+        project = create(:project, path: 'a-project', namespace: existing_group)
+
+        expect(importer).not_to receive(:create_project)
+        expect(importer).to receive(:log).with(" * #{project.name} (#{project_path}) exists")
+
+        importer.create_project_if_needed
+      end
+
+      it 'creates a project with the correct path in the database' do
+        importer.create_project_if_needed
 
-      importer.create_project_if_needed
+        expect(Project.find_by_full_path(project_path)).not_to be_nil
+      end
     end
+  end
+
+  context 'with subgroups', :nested_groups do
+    let(:project_path) { 'a-group/a-sub-group/a-project' }
 
-    it 'skips importing when the project already exists' do
+    let(:existing_group) do
       group = create(:group, path: 'a-group')
-      subgroup = create(:group, path: 'a-sub-group', parent: group)
-      project = create(:project, path: 'a-project', namespace: subgroup)
+      create(:group, path: 'a-sub-group', parent: group)
+    end
 
-      expect(importer).not_to receive(:create_project)
-      expect(importer).to receive(:log).with(" * #{project.name} (a-group/a-sub-group/a-project) exists")
+    it_behaves_like 'importing a repository'
+  end
 
-      importer.create_project_if_needed
-    end
+  context 'without subgroups' do
+    let(:project_path) { 'a-group/a-project' }
+    let(:existing_group) { create(:group, path: 'a-group') }
 
-    it 'creates a project with the correct path in the database' do
-      importer.create_project_if_needed
+    it_behaves_like 'importing a repository'
+  end
+
+  context 'when subgroups are not available' do
+    let(:project_path) { 'a-group/a-sub-group/a-project' }
+
+    before do
+      expect(Group).to receive(:supports_nested_groups?) { false }
+    end
 
-      expect(Project.find_by_full_path(project_path)).not_to be_nil
+    describe '#create_project_if_needed' do
+      it 'raises an error' do
+        expect { importer.create_project_if_needed }.to raise_error('Nested groups are not supported on MySQL')
+      end
     end
   end
 end

spec/lib/gitlab/geo/health_check_spec.rb

@@ -12,6 +12,7 @@ describe Gitlab::Geo::HealthCheck, :postgresql do
       allow(described_class).to receive(:database_secondary?).and_return(true)
       allow(described_class).to receive(:get_database_version).and_return('20170101')
       allow(described_class).to receive(:get_migration_version).and_return('20170201')
+      allow(described_class).to receive(:db_replication_lag).and_return(0)
 
       message = subject.perform_checks
 
@@ -27,8 +28,18 @@ describe Gitlab::Geo::HealthCheck, :postgresql do
 
     it 'returns an error when database is not configured for streaming replication' do
       allow(Gitlab::Geo).to receive(:secondary?) { true }
+      allow(Gitlab::Geo).to receive(:configured?) { true }
       allow(Gitlab::Database).to receive(:postgresql?) { true }
-      allow(ActiveRecord::Base).to receive_message_chain(:connection, :execute, :first, :fetch) { 'f' }
+      allow(described_class).to receive(:database_secondary?) { false }
+
+      expect(subject.perform_checks).not_to be_blank
+    end
+
+    it 'returns an error when streaming replication is not working' do
+      allow(Gitlab::Geo).to receive(:secondary?) { true }
+      allow(Gitlab::Geo).to receive(:configured?) { true }
+      allow(Gitlab::Database).to receive(:postgresql?) { true }
+      allow(described_class).to receive(:database_secondary?) { false }
 
       expect(subject.perform_checks).to include('not configured for streaming replication')
     end
@@ -42,6 +53,7 @@ describe Gitlab::Geo::HealthCheck, :postgresql do
     it 'returns an error when Geo database version does not match the latest migration version' do
       allow(described_class).to receive(:database_secondary?).and_return(true)
       allow(subject).to receive(:get_database_version) { 1 }
+      allow(described_class).to receive(:db_replication_lag).and_return(0)
 
       expect(subject.perform_checks).to match(/Current Geo database version \([0-9]+\) does not match latest migration \([0-9]+\)/)
     end
@@ -49,9 +61,17 @@ describe Gitlab::Geo::HealthCheck, :postgresql do
     it 'returns an error when latest migration version does not match the Geo database version' do
       allow(described_class).to receive(:database_secondary?).and_return(true)
       allow(subject).to receive(:get_migration_version) { 1 }
+      allow(described_class).to receive(:db_replication_lag).and_return(0)
 
       expect(subject.perform_checks).to match(/Current Geo database version \([0-9]+\) does not match latest migration \([0-9]+\)/)
     end
+
+    it 'returns an error when replication lag is not present' do
+      allow(described_class).to receive(:database_secondary?).and_return(true)
+      allow(described_class).to receive(:db_replication_lag).and_return(nil)
+
+      expect(subject.perform_checks).to match(/The Geo node does not appear to be replicating data from the primary node/)
+    end
   end
 
   describe 'MySQL checks' do

spec/lib/gitlab/geo/log_cursor/daemon_spec.rb

 require 'spec_helper'
 
-describe Gitlab::Geo::LogCursor::Daemon do
+describe Gitlab::Geo::LogCursor::Daemon, :postgresql do
   describe '#run!' do
     let!(:geo_node) { create(:geo_node, :current) }
 

spec/lib/gitlab/import_export/project.json

@@ -2967,7 +2967,8 @@
           "action": 1,
           "author_id": 1
         }
-      ]
+      ],
+      "approvals_before_merge": 1
     },
     {
       "id": 26,

spec/lib/gitlab/import_export/project_tree_saver_spec.rb

@@ -33,6 +33,10 @@ describe Gitlab::ImportExport::ProjectTreeSaver do
         expect(saved_project_json).to include({ "visibility_level" => 20 })
       end
 
+      it 'has approvals_before_merge set' do
+        expect(saved_project_json['approvals_before_merge']).to eq(1)
+      end
+
       it 'has milestones' do
         expect(saved_project_json['milestones']).not_to be_empty
       end
@@ -241,7 +245,8 @@ describe Gitlab::ImportExport::ProjectTreeSaver do
                      issues: [issue],
                      snippets: [snippet],
                      releases: [release],
-                     group: group
+                     group: group,
+                     approvals_before_merge: 1
                     )
     project.update_column(:description_html, 'description')
     project_label = create(:label, project: project)

spec/models/geo_node_status_spec.rb

@@ -98,6 +98,14 @@ describe GeoNodeStatus do
     end
   end
 
+  describe '#db_replication_lag' do
+    it 'returns the set replication lag' do
+      allow(Gitlab::Geo::HealthCheck).to receive(:db_replication_lag).and_return(1000)
+
+      expect(subject.db_replication_lag).to eq(1000)
+    end
+  end
+
   describe '#lfs_objects_synced_in_percentage' do
     let(:lfs_object_project) { create(:lfs_objects_project, project: project_1) }
 
@@ -164,6 +172,7 @@ describe GeoNodeStatus do
 
   context 'when no values are available' do
     it 'returns 0 for each attribute' do
+      allow(Gitlab::Geo::HealthCheck).to receive(:db_replication_lag).and_return(nil)
       subject.attachments_count = nil
       subject.attachments_synced_count = nil
       subject.lfs_objects_count = nil
@@ -172,6 +181,7 @@ describe GeoNodeStatus do
       subject.repositories_synced_count = nil
       subject.repositories_failed_count = nil
 
+      expect(subject.db_replication_lag).to be_nil
       expect(subject.repositories_count).to be_zero
       expect(subject.repositories_synced_count).to be_zero
       expect(subject.repositories_synced_in_percentage).to be_zero

spec/services/geo/node_status_service_spec.rb

@@ -25,6 +25,7 @@ describe Geo::NodeStatusService do
 
     it 'parses a 200 response' do
       data = { health: 'OK',
+               db_replication_lag: 0,
                repositories_count: 10,
                repositories_synced_count: 1,
                repositories_failed_count: 2,

spec/services/groups/nested_create_service_spec.rb

@@ -2,52 +2,87 @@ require 'spec_helper'
 
 describe Groups::NestedCreateService do
   let(:user) { create(:user) }
-  let(:params) { { group_path: 'a-group/a-sub-group' } }
 
   subject(:service) { described_class.new(user, params) }
 
-  describe "#execute" do
-    it 'returns the group if it already existed' do
-      parent = create(:group, path: 'a-group', owner: user)
-      child = create(:group, path: 'a-sub-group', parent: parent, owner: user)
+  shared_examples 'with a visibility level' do
+    it 'creates the group with correct visibility level' do
+      allow(Gitlab::CurrentSettings.current_application_settings)
+        .to receive(:default_group_visibility) { Gitlab::VisibilityLevel::INTERNAL }
+
+      group = service.execute
 
-      expect(service.execute).to eq(child)
+      expect(group.visibility_level).to eq(Gitlab::VisibilityLevel::INTERNAL)
     end
 
-    it 'reuses a parent if it already existed', :nested_groups do
-      parent = create(:group, path: 'a-group')
-      parent.add_owner(user)
+    context 'adding a visibility level ' do
+      it 'overwrites the visibility level' do
+        service = described_class.new(user, params.merge(visibility_level: Gitlab::VisibilityLevel::PRIVATE))
+
+        group = service.execute
 
-      expect(service.execute.parent).to eq(parent)
+        expect(group.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE)
+      end
     end
+  end
+
+  describe 'without subgroups' do
+    let(:params) { { group_path: 'a-group' } }
 
-    it 'creates group and subgroup in the database', :nested_groups do
-      service.execute
+    before do
+      allow(Group).to receive(:supports_nested_groups?) { false }
+    end
 
-      parent = Group.find_by_full_path('a-group')
-      child = parent.children.find_by(path: 'a-sub-group')
+    it 'creates the group' do
+      group = service.execute
 
-      expect(parent).not_to be_nil
-      expect(child).not_to be_nil
+      expect(group).to be_persisted
     end
 
-    it 'creates the group with correct visibility level' do
-      allow(Gitlab::CurrentSettings.current_application_settings)
-        .to receive(:default_group_visibility) { Gitlab::VisibilityLevel::INTERNAL }
+    it 'returns the group if it already existed' do
+      existing_group = create(:group, path: 'a-group')
 
-      group = service.execute
+      expect(service.execute).to eq(existing_group)
+    end
 
-      expect(group.visibility_level).to eq(Gitlab::VisibilityLevel::INTERNAL)
+    it 'raises an error when tring to create a subgroup' do
+      service = described_class.new(user, group_path: 'a-group/a-sub-group')
+
+      expect { service.execute }.to raise_error('Nested groups are not supported on MySQL')
     end
 
-    context 'adding a visibility level ' do
-      let(:params) { { group_path: 'a-group/a-sub-group', visibility_level: Gitlab::VisibilityLevel::PRIVATE } }
+    it_behaves_like 'with a visibility level'
+  end
 
-      it 'overwrites the visibility level' do
-        group = service.execute
+  describe 'with subgroups', :nested_groups do
+    let(:params) { { group_path: 'a-group/a-sub-group' } }
 
-        expect(group.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE)
+    describe "#execute" do
+      it 'returns the group if it already existed' do
+        parent = create(:group, path: 'a-group', owner: user)
+        child = create(:group, path: 'a-sub-group', parent: parent, owner: user)
+
+        expect(service.execute).to eq(child)
       end
+
+      it 'reuses a parent if it already existed' do
+        parent = create(:group, path: 'a-group')
+        parent.add_owner(user)
+
+        expect(service.execute.parent).to eq(parent)
+      end
+
+      it 'creates group and subgroup in the database' do
+        service.execute
+
+        parent = Group.find_by_full_path('a-group')
+        child = parent.children.find_by(path: 'a-sub-group')
+
+        expect(parent).not_to be_nil
+        expect(child).not_to be_nil
+      end
+
+      it_behaves_like 'with a visibility level'
     end
   end
 end