Merge branch 'ce-to-ee-2018-04-03' into 'master'

CE upstream - 2018-04-03 18:27 UTC

See merge request gitlab-org/gitlab-ee!5216

.gitlab-ci.yml

@@ -709,6 +709,9 @@ karma:
 codequality:
   <<: *dedicated-no-docs-no-db-pull-cache-job
   image: docker:latest
+  # gitlab-org runners set `privileged: false` but we need to have it set to true
+  # since we're using Docker in Docker
+  tags: []
   before_script: []
   services:
     - docker:dind

.gitlab/merge_request_templates/Database Changes.md

@@ -33,13 +33,16 @@ When removing columns, tables, indexes or other structures:
 
 ## General Checklist
 
-- [ ] [Changelog entry](https://docs.gitlab.com/ce/development/changelog.html) added, if necessary
-- [ ] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
+- [ ] [Changelog entry](https://docs.gitlab.com/ee/development/changelog.html) added, if necessary
+- [ ] [Documentation created/updated](https://docs.gitlab.com/ee/development/doc_styleguide.html)
 - [ ] API support added
 - [ ] Tests added for this feature/bug
 - Review
   - [ ] Has been reviewed by Backend
   - [ ] Has been reviewed by Database
-- [ ] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
-- [ ] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
+- [ ] Conform by the [merge request performance guides](https://docs.gitlab.com/ee/development/merge_request_performance_guidelines.html)
+- [ ] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ee/blob/master/CONTRIBUTING.md#style-guides)
 - [ ] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)
+- [ ] Internationalization required/considered
+- [ ] If paid feature, have we considered GitLab.com plan and how it works for groups and is there a design for promoting it to users who aren't on the correct plan
+- [ ] End-to-end tests pass (`package-qa` manual pipeline job)

GITALY_SERVER_VERSION

-0.92.0
+0.93.0

app/controllers/projects/branches_controller.rb

@@ -22,9 +22,13 @@ class Projects::BranchesController < Projects::ApplicationController
 
         @refs_pipelines = @project.pipelines.latest_successful_for_refs(@branches.map(&:name))
         @merged_branch_names = repository.merged_branch_names(@branches.map(&:name))
-        @max_commits = @branches.reduce(0) do |memo, branch|
-          diverging_commit_counts = repository.diverging_commit_counts(branch)
-          [memo, diverging_commit_counts[:behind], diverging_commit_counts[:ahead]].max
+
+        # n+1: https://gitlab.com/gitlab-org/gitaly/issues/992
+        Gitlab::GitalyClient.allow_n_plus_1_calls do
+          @max_commits = @branches.reduce(0) do |memo, branch|
+            diverging_commit_counts = repository.diverging_commit_counts(branch)
+            [memo, diverging_commit_counts[:behind], diverging_commit_counts[:ahead]].max
+          end
         end
 
         render

app/mailers/emails/merge_requests.rb

@@ -15,6 +15,7 @@ module Emails
       setup_merge_request_mail(merge_request_id, recipient_id)
       @new_commits = new_commits
       @existing_commits = existing_commits
+      @updated_by_user = User.find(updated_by_user_id)
 
       mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason))
     end

app/services/projects/update_pages_service.rb

@@ -31,15 +31,17 @@ module Projects
 
         # Check if we did extract public directory
         archive_public_path = File.join(archive_path, 'public')
-        raise FailedToExtractError, 'pages miss the public folder' unless Dir.exist?(archive_public_path)
+        raise InvaildStateError, 'pages miss the public folder' unless Dir.exist?(archive_public_path)
         raise InvaildStateError, 'pages are outdated' unless latest?
 
         deploy_page!(archive_public_path)
         success
       end
-    rescue InvaildStateError, FailedToExtractError => e
-      register_failure
+    rescue InvaildStateError => e
       error(e.message)
+    rescue => e
+      error(e.message, false)
+      raise e
     end
 
     private
@@ -50,12 +52,13 @@ module Projects
       super
     end
 
-    def error(message, http_status = nil)
+    def error(message, allow_delete_artifact = true)
+      register_failure
       log_error("Projects::UpdatePagesService: #{message}")
       @status.allow_failure = !latest?
       @status.description = message
       @status.drop(:script_failure)
-      delete_artifact!
+      delete_artifact! if allow_delete_artifact
       super
     end
 
@@ -76,7 +79,7 @@ module Projects
       elsif artifacts_filename.ends_with?('.zip')
         extract_zip_archive!(temp_path)
       else
-        raise FailedToExtractError, 'unsupported artifacts format'
+        raise InvaildStateError, 'unsupported artifacts format'
       end
     end
 
@@ -91,13 +94,13 @@ module Projects
     end
 
     def extract_zip_archive!(temp_path)
-      raise FailedToExtractError, 'missing artifacts metadata' unless build.artifacts_metadata?
+      raise InvaildStateError, 'missing artifacts metadata' unless build.artifacts_metadata?
 
       # Calculate page size after extract
       public_entry = build.artifacts_metadata_entry(SITE_PATH, recursive: true)
 
       if public_entry.total_size > max_size
-        raise FailedToExtractError, "artifacts for pages are too large: #{public_entry.total_size}"
+        raise InvaildStateError, "artifacts for pages are too large: #{public_entry.total_size}"
       end
 
       # Requires UnZip at least 6.00 Info-ZIP.

app/views/notify/push_to_merge_request_email.html.haml

 %h3
-  New commits were pushed to the merge request
+  = @updated_by_user.name
+  pushed new commits to merge request
   = link_to(@merge_request.to_reference, project_merge_request_url(@merge_request.target_project, @merge_request))
-  by #{@current_user.name}
 
 - if @existing_commits.any?
   - count = @existing_commits.size

app/views/notify/push_to_merge_request_email.text.haml

-New commits were pushed to the merge request #{@merge_request.to_reference} by #{@current_user.name}
+#{@updated_by_user.name} pushed new commits to merge request #{@merge_request.to_reference}
 \
 #{url_for(project_merge_request_url(@merge_request.target_project, @merge_request))}
 \

bin/rspec

 #!/usr/bin/env ruby
+
+# Remove these two lines below when upgraded to rails 5.0.
+# Allow run `rspec` command as `RAILS5=1 rspec ...` instead of `BUNDLE_GEMFILE=Gemfile.rails5 rspec ...`
+gemfile = %w[1 true].include?(ENV["RAILS5"]) ? "Gemfile.rails5" : "Gemfile"
+ENV['BUNDLE_GEMFILE'] ||= File.expand_path("../#{gemfile}", __dir__)
+
 begin
   load File.expand_path('../spring', __FILE__)
 rescue LoadError => e

changelogs/unreleased/43745-store-metadata-checksum-for-artifacts.yml

+---
+title: Store sha256 checksum of artifact metadata
+merge_request: 18149
+author:
+type: added

changelogs/unreleased/44425-use-gitlab_environment.yml

+---
+title: Fix `gitlab-rake gitlab:two_factor:disable_for_all_users`
+merge_request: 18154
+author:
+type: fixed

changelogs/unreleased/44902-remove-rake-test-ci.yml

+---
+title: Remove test_ci rake task
+merge_request: 18139
+author: Takuya Noguchi
+type: other

changelogs/unreleased/blackst0ne-replace-spinach-project-issues-labels-feature.yml

+---
+title: Replace the `project/issues/labels.feature` spinach test with an rspec analog
+merge_request: 18126
+author: blackst0ne
+type: other

changelogs/unreleased/zj-bump-gitaly.yml

+---
+title: Upgrade Gitaly to upgrade its charlock_holmes
+merge_request:
+author:
+type: other

doc/development/ee_features.md

@@ -644,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,
@@ -683,6 +684,19 @@ to avoid conflicts during CE to EE merge.
 // 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

features/project/issues/labels.feature

-@project_issues
-Feature: Project Issues Labels
-  Background:
-    Given I sign in as a user
-    And I own project "Shop"
-    And project "Shop" has labels: "bug", "feature", "enhancement"
-    Given I visit project "Shop" labels page
-
-  Scenario: I should see labels list
-    Then I should see label 'bug'
-    And I should see label 'feature'
-
-  Scenario: I create new label
-    Given I visit project "Shop" new label page
-    When I submit new label 'support'
-    Then I should see label 'support'
-
-  Scenario: I edit label
-    Given I visit 'bug' label edit page
-    When I change label 'bug' to 'fix'
-    Then I should not see label 'bug'
-    Then I should see label 'fix'
-
-  Scenario: I remove label
-    When I remove label 'bug'
-    Then I should not see label 'bug'
-
-  @javascript
-  Scenario: I remove all labels
-    When I delete all labels
-    Then I should see labels help message
-
-  Scenario: I create a label with invalid color
-    Given I visit project "Shop" new label page
-    When I submit new label with invalid color
-    Then I should see label color error message
-
-  Scenario: I create a label that already exists
-    Given I visit project "Shop" new label page
-    When I submit new label 'bug'
-    Then I should see label label exist error message
-
-  Scenario: I create the same label on another project
-    Given I own project "Forum"
-    And I visit project "Forum" labels page
-    And I visit project "Forum" new label page
-    When I submit new label 'bug'
-    Then I should see label 'bug'

features/steps/project/issues/labels.rb

-class Spinach::Features::ProjectIssuesLabels < Spinach::FeatureSteps
-  include SharedAuthentication
-  include SharedProject
-  include SharedPaths
-
-  step 'I visit \'bug\' label edit page' do
-    visit edit_project_label_path(project, bug_label)
-  end
-
-  step 'I remove label \'bug\'' do
-    page.within "#project_label_#{bug_label.id}" do
-      first(:link, 'Delete').click
-    end
-  end
-
-  step 'I delete all labels' do
-    page.within '.labels' do
-      page.all('.label-list-item').each do
-        first('.remove-row').click
-        first(:link, 'Delete label').click
-      end
-    end
-  end
-
-  step 'I should see labels help message' do
-    page.within '.labels' do
-      expect(page).to have_content 'Generate a default set of labels'
-      expect(page).to have_content 'New label'
-    end
-  end
-
-  step 'I submit new label \'support\'' do
-    fill_in 'Title', with: 'support'
-    fill_in 'Background color', with: '#F95610'
-    click_button 'Create label'
-  end
-
-  step 'I submit new label \'bug\'' do
-    fill_in 'Title', with: 'bug'
-    fill_in 'Background color', with: '#F95610'
-    click_button 'Create label'
-  end
-
-  step 'I submit new label with invalid color' do
-    fill_in 'Title', with: 'support'
-    fill_in 'Background color', with: '#12'
-    click_button 'Create label'
-  end
-
-  step 'I should see label label exist error message' do
-    page.within '.label-form' do
-      expect(page).to have_content 'Title has already been taken'
-    end
-  end
-
-  step 'I should see label color error message' do
-    page.within '.label-form' do
-      expect(page).to have_content 'Color must be a valid color code'
-    end
-  end
-
-  step 'I should see label \'feature\'' do
-    page.within '.other-labels .manage-labels-list' do
-      expect(page).to have_content 'feature'
-    end
-  end
-
-  step 'I should see label \'bug\'' do
-    page.within '.other-labels .manage-labels-list' do
-      expect(page).to have_content 'bug'
-    end
-  end
-
-  step 'I should not see label \'bug\'' do
-    page.within '.other-labels .manage-labels-list' do
-      expect(page).not_to have_content 'bug'
-    end
-  end
-
-  step 'I should see label \'support\'' do
-    page.within '.other-labels .manage-labels-list' do
-      expect(page).to have_content 'support'
-    end
-  end
-
-  step 'I change label \'bug\' to \'fix\'' do
-    fill_in 'Title', with: 'fix'
-    fill_in 'Background color', with: '#F15610'
-    click_button 'Save changes'
-  end
-
-  step 'I should see label \'fix\'' do
-    page.within '.other-labels .manage-labels-list' do
-      expect(page).to have_content 'fix'
-    end
-  end
-
-  def bug_label
-    project.labels.find_or_create_by(title: 'bug')
-  end
-end

features/steps/shared/paths.rb

@@ -356,11 +356,6 @@ module SharedPaths
     visit project_issue_path(issue.project, issue)
   end
 
-  step 'I visit project "Shop" labels page' do
-    project = Project.find_by(name: 'Shop')
-    visit project_labels_path(project)
-  end
-
   step 'I visit project "Forum" labels page' do
     project = Project.find_by(name: 'Forum')
     visit project_labels_path(project)

lib/api/runner.rb

@@ -208,6 +208,7 @@ module API
         optional 'file.sha256', type: String, desc: %q(sha256 checksum of the file)
         optional 'metadata.path', type: String, desc: %q(path to locally stored body (generated by Workhorse))
         optional 'metadata.name', type: String, desc: %q(filename (generated by Workhorse))
+        optional 'metadata.sha256', type: String, desc: %q(sha256 checksum of the file)
       end
       post '/:id/artifacts' do
         not_allowed! unless Gitlab.config.artifacts.enabled
@@ -227,7 +228,7 @@ module API
           Gitlab::CurrentSettings.current_application_settings.default_artifacts_expire_in
 
         job.build_job_artifacts_archive(project: job.project, file_type: :archive, file: artifacts, file_sha256: params['file.sha256'], expire_in: expire_in)
-        job.build_job_artifacts_metadata(project: job.project, file_type: :metadata, file: metadata, expire_in: expire_in) if metadata
+        job.build_job_artifacts_metadata(project: job.project, file_type: :metadata, file: metadata, file_sha256: params['metadata.sha256'], expire_in: expire_in) if metadata
         job.artifacts_expire_in = expire_in
 
         if job.save

lib/gitlab/performance_bar.rb

@@ -5,6 +5,7 @@ module Gitlab
 
     def self.enabled?(user = nil)
       return true if Rails.env.development?
+      return true if user&.admin?
       return false unless user && allowed_group_id
 
       allowed_user_ids.include?(user.id)

lib/tasks/gitlab/two_factor.rake

 namespace :gitlab do
   namespace :two_factor do
     desc "GitLab | Disable Two-factor authentication (2FA) for all users"
-    task disable_for_all_users: :environment do
+    task disable_for_all_users: :gitlab_environment do
       scope = User.with_two_factor
       count = scope.count
 

lib/tasks/test.rake

@@ -4,8 +4,3 @@ desc "GitLab | Run all tests"
 task :test do
   Rake::Task["gitlab:test"].invoke
 end
-
-unless Rails.env.production?
-  desc "GitLab | Run all tests on CI with simplecov"
-  task test_ci: [:rubocop, :brakeman, :karma, :spinach, :spec]
-end

spec/controllers/projects/branches_controller_spec.rb

@@ -416,6 +416,22 @@ describe Projects::BranchesController do
       end
     end
 
+    # We need :request_store because Gitaly only counts the queries whenever
+    # `RequestStore.active?` in GitalyClient.enforce_gitaly_request_limits
+    # And the main goal of this test is making sure TooManyInvocationsError
+    # was not raised whenever the cache is enabled yet cold.
+    context 'when cache is enabled yet cold', :request_store do
+      it 'return with a status 200' do
+        get :index,
+            namespace_id: project.namespace,
+            project_id: project,
+            state: 'all',
+            format: :html
+
+        expect(response).to have_gitlab_http_status(200)
+      end
+    end
+
     context 'when branch contains an invalid UTF-8 sequence' do
       before do
         project.repository.create_branch("wrong-\xE5-utf8-sequence")
@@ -432,7 +448,7 @@ describe Projects::BranchesController do
       end
     end
 
-    context 'when depreated sort/search/page parameters are specified' do
+    context 'when deprecated sort/search/page parameters are specified' do
       it 'returns with a status 301 when sort specified' do
         get :index,
             namespace_id: project.namespace,

spec/features/projects/labels/user_creates_labels_spec.rb

+require "spec_helper"
+
+describe "User creates labels" do
+  set(:project) { create(:project_empty_repo, :public) }
+  set(:user) { create(:user) }
+
+  shared_examples_for "label creation" do
+    it "creates new label" do
+      title = "bug"
+
+      create_label(title)
+
+      page.within(".other-labels .manage-labels-list") do
+        expect(page).to have_content(title)
+      end
+    end
+  end
+
+  context "in project" do
+    before do
+      project.add_master(user)
+      sign_in(user)
+
+      visit(new_project_label_path(project))
+    end
+
+    context "when data is valid" do
+      include_examples "label creation"
+    end
+
+    context "when data is invalid" do
+      context "when title is invalid" do
+        it "shows error message" do
+          create_label("")
+
+          page.within(".label-form") do
+            expect(page).to have_content("Title can't be blank")
+          end
+        end
+      end
+
+      context "when color is invalid" do
+        it "shows error message" do
+          create_label("feature", "#12")
+
+          page.within(".label-form") do
+            expect(page).to have_content("Color must be a valid color code")
+          end
+        end
+      end
+    end
+
+    context "when label already exists" do
+      let!(:label) { create(:label, project: project) }
+
+      it "shows error message" do
+        create_label(label.title)
+
+        page.within(".label-form") do
+          expect(page).to have_content("Title has already been taken")
+        end
+      end
+    end
+  end
+
+  context "in another project" do
+    set(:another_project) { create(:project_empty_repo, :public) }
+
+    before do
+      create(:label, project: project, title: "bug") # Create label for `project` (not `another_project`) project.
+
+      another_project.add_master(user)
+      sign_in(user)
+
+      visit(new_project_label_path(another_project))
+    end
+
+    include_examples "label creation"
+  end
+
+  private
+
+  def create_label(title, color = "#F95610")
+    fill_in("Title", with: title)
+    fill_in("Background color", with: color)
+    click_button("Create label")
+  end
+end

spec/features/projects/labels/user_edits_labels_spec.rb

+require "spec_helper"
+
+describe "User edits labels" do
+  set(:project) { create(:project_empty_repo, :public) }
+  set(:label) { create(:label, project: project) }
+  set(:user) { create(:user) }
+
+  before do
+    project.add_master(user)
+    sign_in(user)
+
+    visit(edit_project_label_path(project, label))
+  end
+
+  it "updates label's title" do
+    new_title = "fix"
+
+    fill_in("Title", with: new_title)
+    click_button("Save changes")
+
+    page.within(".other-labels .manage-labels-list") do
+      expect(page).to have_content(new_title).and have_no_content(label.title)
+    end
+  end
+end

spec/features/projects/labels/user_removes_labels_spec.rb

+require "spec_helper"
+
+describe "User removes labels" do
+  let(:project) { create(:project_empty_repo, :public) }
+  let(:user) { create(:user) }
+
+  before do
+    project.add_master(user)
+    sign_in(user)
+  end
+
+  context "when one label" do
+    let!(:label) { create(:label, project: project) }
+
+    before do
+      visit(project_labels_path(project))
+    end
+
+    it "removes label" do
+      page.within(".labels") do
+        page.first(".label-list-item") do
+          first(".remove-row").click
+          first(:link, "Delete label").click
+        end
+      end
+
+      expect(page).to have_content("Label was removed").and have_no_content(label.title)
+    end
+  end
+
+  context "when many labels", :js do
+    before do
+      create_list(:label, 3, project: project)
+
+      visit(project_labels_path(project))
+    end
+
+    it "removes all labels" do
+      page.within(".labels") do
+        loop do
+          li = page.first(".label-list-item")
+          break unless li
+
+          li.click_link("Delete")
+          click_link("Delete label")
+        end
+
+        expect(page).to have_content("Generate a default set of labels").and have_content("New label")
+      end
+    end
+  end
+end

spec/features/projects/labels/user_views_labels_spec.rb

+require "spec_helper"
+
+describe "User views labels" do
+  set(:project) { create(:project_empty_repo, :public) }
+  set(:user) { create(:user) }
+
+  LABEL_TITLES = %w[bug enhancement feature].freeze
+
+  before do
+    LABEL_TITLES.each { |title| create(:label, project: project, title: title) }
+
+    project.add_guest(user)
+    sign_in(user)
+
+    visit(project_labels_path(project))
+  end
+
+  it "shows all labels" do
+    page.within('.other-labels .manage-labels-list') do
+      LABEL_TITLES.each { |title| expect(page).to have_content(title) }
+    end
+  end
+end

spec/lib/gitlab/performance_bar_spec.rb

@@ -25,6 +25,12 @@ describe Gitlab::PerformanceBar do
       expect(described_class.enabled?(nil)).to be_falsy
     end
 
+    it 'returns true when given user is an admin' do
+      user = build_stubbed(:user, :admin)
+
+      expect(described_class.enabled?(user)).to be_truthy
+    end
+
     it 'returns false when allowed_group_id is nil' do
       expect(described_class).to receive(:allowed_group_id).and_return(nil)
 

spec/mailers/notify_spec.rb

@@ -400,6 +400,36 @@ describe Notify do
           end
         end
       end
+
+      describe 'that have new commits' do
+        let(:push_user) { create(:user) }
+
+        subject do
+          described_class.push_to_merge_request_email(recipient.id, merge_request.id, push_user.id, new_commits: merge_request.commits)
+        end
+
+        it_behaves_like 'a multiple recipients email'
+        it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
+          let(:model) { merge_request }
+        end
+        it_behaves_like 'it should show Gmail Actions View Merge request link'
+        it_behaves_like 'an unsubscribeable thread'
+
+        it 'is sent as the push user' do
+          sender = subject.header[:from].addrs[0]
+
+          expect(sender.display_name).to eq(push_user.name)
+          expect(sender.address).to eq(gitlab_sender)
+        end
+
+        it 'has the correct subject and body' do
+          aggregate_failures do
+            is_expected.to have_referable_subject(merge_request, reply: true)
+            is_expected.to have_body_text("#{push_user.name} pushed new commits")
+            is_expected.to have_body_text(project_merge_request_path(project, merge_request))
+          end
+        end
+      end
     end
 
     context 'for issue notes' do

spec/requests/api/runner_spec.rb

@@ -1161,11 +1161,13 @@ describe API::Runner do
             let!(:artifacts) { file_upload }
             let!(:artifacts_sha256) { Digest::SHA256.file(artifacts.path).hexdigest }
             let!(:metadata) { file_upload2 }
+            let!(:metadata_sha256) { Digest::SHA256.file(metadata.path).hexdigest }
 
             let(:stored_artifacts_file) { job.reload.artifacts_file.file }
             let(:stored_metadata_file) { job.reload.artifacts_metadata.file }
             let(:stored_artifacts_size) { job.reload.artifacts_size }
             let(:stored_artifacts_sha256) { job.reload.job_artifacts_archive.file_sha256 }
+            let(:stored_metadata_sha256) { job.reload.job_artifacts_metadata.file_sha256 }
 
             before do
               post(api("/jobs/#{job.id}/artifacts"), post_data, headers_with_token)
@@ -1177,7 +1179,8 @@ describe API::Runner do
                   'file.name' => artifacts.original_filename,
                   'file.sha256' => artifacts_sha256,
                   'metadata.path' => metadata.path,
-                  'metadata.name' => metadata.original_filename }
+                  'metadata.name' => metadata.original_filename,
+                  'metadata.sha256' => metadata_sha256 }
               end
 
               it 'stores artifacts and artifacts metadata' do
@@ -1186,6 +1189,7 @@ describe API::Runner do
                 expect(stored_metadata_file.original_filename).to eq(metadata.original_filename)
                 expect(stored_artifacts_size).to eq(72821)
                 expect(stored_artifacts_sha256).to eq(artifacts_sha256)
+                expect(stored_metadata_sha256).to eq(metadata_sha256)
               end
             end
 

spec/services/projects/update_pages_service_spec.rb

@@ -87,7 +87,8 @@ describe Projects::UpdatePagesService do
         it 'fails for empty file fails' do
           build.update_attributes(legacy_artifacts_file: empty_file)
 
-          expect(execute).not_to eq(:success)
+          expect { execute }
+            .to raise_error(Projects::UpdatePagesService::FailedToExtractError)
         end
       end
     end
@@ -159,7 +160,8 @@ describe Projects::UpdatePagesService do
       it 'fails for empty file fails' do
         build.job_artifacts_archive.update_attributes(file: empty_file)
 
-        expect(execute).not_to eq(:success)
+        expect { execute }
+          .to raise_error(Projects::UpdatePagesService::FailedToExtractError)
       end
 
       context 'when timeout happens by DNS error' do
@@ -172,7 +174,39 @@ describe Projects::UpdatePagesService do
           expect { execute }.to raise_error(SocketError)
 
           build.reload
-          expect(build.artifacts?).to eq(true)
+          expect(deploy_status).to be_failed
+          expect(build.artifacts?).to be_truthy
+        end
+      end
+
+      context 'when failed to extract zip artifacts' do
+        before do
+          allow_any_instance_of(described_class)
+            .to receive(:extract_zip_archive!)
+            .and_raise(Projects::UpdatePagesService::FailedToExtractError)
+        end
+
+        it 'raises an error' do
+          expect { execute }
+            .to raise_error(Projects::UpdatePagesService::FailedToExtractError)
+
+          build.reload
+          expect(deploy_status).to be_failed
+          expect(build.artifacts?).to be_truthy
+        end
+      end
+
+      context 'when missing artifacts metadata' do
+        before do
+          allow(build).to receive(:artifacts_metadata?).and_return(false)
+        end
+
+        it 'does not raise an error and remove artifacts as failed job' do
+          execute
+
+          build.reload
+          expect(deploy_status).to be_failed
+          expect(build.artifacts?).to be_falsey
         end
       end
     end