Merge branch '11-5-stable-prepare-rc10' into '11-5-stable'

Prepare 11.5 RC10 release

See merge request gitlab-org/gitlab-ce!23116

12 files changed, 192 insertions, 11 deletions

diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue
index e76c7afd863..93834e72417 100644
--- a/app/assets/javascripts/diffs/components/diff_file.vue
+++ b/app/assets/javascripts/diffs/components/diff_file.vue
@@ -32,6 +32,7 @@ export default {
   computed: {
     ...mapState('diffs', ['currentDiffFileId']),
     ...mapGetters(['isNotesFetched']),
+    ...mapGetters('diffs', ['getDiffFileDiscussions']),
     isCollapsed() {
       return this.file.collapsed || false;
     },
@@ -57,12 +58,23 @@ export default {
     showLoadingIcon() {
       return this.isLoadingCollapsedDiff || (!this.file.renderIt && !this.isCollapsed);
     },
+    hasDiffLines() {
+      const { highlightedDiffLines, parallelDiffLines } = this.file;
+
+      return highlightedDiffLines && parallelDiffLines && parallelDiffLines.length > 0;
+    },
+  },
+  watch: {
+    'file.collapsed': function fileCollapsedWatch(newVal, oldVal) {
+      if (!newVal && oldVal && !this.hasDiffLines) {
+        this.handleLoadCollapsedDiff();
+      }
+    },
   },
   methods: {
     ...mapActions('diffs', ['loadCollapsedDiff', 'assignDiscussionsToDiff']),
     handleToggle() {
-      const { highlightedDiffLines, parallelDiffLines } = this.file;
-      if (!highlightedDiffLines && parallelDiffLines !== undefined && !parallelDiffLines.length) {
+      if (!this.hasDiffLines) {
         this.handleLoadCollapsedDiff();
       } else {
         this.file.collapsed = !this.file.collapsed;
@@ -81,7 +93,7 @@ export default {
         .then(() => {
           requestIdleCallback(
             () => {
-              this.assignDiscussionsToDiff();
+              this.assignDiscussionsToDiff(this.getDiffFileDiscussions(this.file));
             },
             { timeout: 1000 },
           );
diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss
index e4f59778d1e..1f34537d856 100644
--- a/app/assets/stylesheets/pages/notes.scss
+++ b/app/assets/stylesheets/pages/notes.scss
@@ -272,7 +272,7 @@ $note-form-margin-left: 72px;
   }
 
   .system-note {
-    padding: 6px 20px;
+    padding: 6px 21px;
     margin: $gl-padding-24 0;
     background-color: transparent;
 
@@ -407,6 +407,24 @@ $note-form-margin-left: 72px;
   }
 }
 
+.tab-pane.notes {
+  .diff-file .notes .system-note {
+    margin: 0;
+  }
+}
+
+.tab-pane.diffs {
+  .system-note {
+    padding: 0 $gl-padding;
+    margin-left: 20px;
+  }
+
+  .notes > .note-discussion li.note.system-note {
+    border-bottom: 0;
+    padding: 0 $gl-padding;
+  }
+}
+
 .diff-file {
   .is-over {
     .add-diff-note {
@@ -426,7 +444,7 @@ $note-form-margin-left: 72px;
     }
 
     .system-note {
-      margin: 0;
+      background-color: $white-light;
       padding: $gl-padding;
     }
   }
@@ -485,6 +503,11 @@ $note-form-margin-left: 72px;
 
   .note-wrapper {
     @include outline-comment();
+
+    &.system-note {
+      border: 0;
+      margin-left: 20px;
+    }
   }
 
   .discussion-reply-holder {
diff --git a/app/models/concerns/deployable.rb b/app/models/concerns/deployable.rb
index 85db01af18d..bc12b06b5af 100644
--- a/app/models/concerns/deployable.rb
+++ b/app/models/concerns/deployable.rb
@@ -13,6 +13,10 @@ module Deployable
         name: expanded_environment_name
       )
 
+      # If we failed to persist envirionment record by validation error, such as name with invalid character,
+      # the job will fall back to a non-environment job.
+      return unless environment.persisted?
+
       create_deployment!(
         project_id: environment.project_id,
         environment: environment,
diff --git a/app/models/deployment.rb b/app/models/deployment.rb
index 83434276995..811e623b7f7 100644
--- a/app/models/deployment.rb
+++ b/app/models/deployment.rb
@@ -160,18 +160,18 @@ class Deployment < ActiveRecord::Base
   end
 
   def has_metrics?
-    prometheus_adapter&.can_query?
+    prometheus_adapter&.can_query? && success?
   end
 
   def metrics
-    return {} unless has_metrics? && success?
+    return {} unless has_metrics?
 
     metrics = prometheus_adapter.query(:deployment, self)
     metrics&.merge(deployment_time: finished_at.to_i) || {}
   end
 
   def additional_metrics
-    return {} unless has_metrics? && success?
+    return {} unless has_metrics?
 
     metrics = prometheus_adapter.query(:additional_metrics_deployment, self)
     metrics&.merge(deployment_time: finished_at.to_i) || {}
diff --git a/app/serializers/environment_status_entity.rb b/app/serializers/environment_status_entity.rb
index f87cc894d2f..19c1df25e18 100644
--- a/app/serializers/environment_status_entity.rb
+++ b/app/serializers/environment_status_entity.rb
@@ -11,7 +11,7 @@ class EnvironmentStatusEntity < Grape::Entity
     project_environment_path(es.project, es.environment)
   end
 
-  expose :metrics_url, if: ->(*) { can_read_environment? && environment.has_metrics? } do |es|
+  expose :metrics_url, if: ->(*) { can_read_environment? && deployment.has_metrics? } do |es|
     metrics_project_environment_deployment_path(es.project, es.environment, es.deployment)
   end
 
@@ -45,6 +45,10 @@ class EnvironmentStatusEntity < Grape::Entity
     object.environment
   end
 
+  def deployment
+    object.deployment
+  end
+
   def project
     object.environment.project
   end
diff --git a/changelogs/unreleased/fix-deployment-metrics-in-mr-widget.yml b/changelogs/unreleased/fix-deployment-metrics-in-mr-widget.yml
new file mode 100644
index 00000000000..5427ead3d1b
--- /dev/null
+++ b/changelogs/unreleased/fix-deployment-metrics-in-mr-widget.yml
@@ -0,0 +1,6 @@
+---
+title: Avoid returning deployment metrics url to MR widget when the deployment is
+  not successful
+merge_request: 23010
+author:
+type: fixed
diff --git a/changelogs/unreleased/ignore-environment-validation-failure.yml b/changelogs/unreleased/ignore-environment-validation-failure.yml
new file mode 100644
index 00000000000..1b61cf86dc4
--- /dev/null
+++ b/changelogs/unreleased/ignore-environment-validation-failure.yml
@@ -0,0 +1,5 @@
+---
+title: Ignore environment validation failure
+merge_request: 23100
+author:
+type: fixed
diff --git a/spec/javascripts/diffs/components/diff_file_spec.js b/spec/javascripts/diffs/components/diff_file_spec.js
index 882ad3685a2..431944eee96 100644
--- a/spec/javascripts/diffs/components/diff_file_spec.js
+++ b/spec/javascripts/diffs/components/diff_file_spec.js
@@ -107,4 +107,26 @@ describe('DiffFile', () => {
       });
     });
   });
+
+  describe('watch collapsed', () => {
+    it('calls handleLoadCollapsedDiff if collapsed changed & file has no lines', done => {
+      spyOn(vm, 'handleLoadCollapsedDiff');
+
+      vm.file.highlightedDiffLines = undefined;
+      vm.file.parallelDiffLines = [];
+      vm.file.collapsed = true;
+
+      vm.$nextTick()
+        .then(() => {
+          vm.file.collapsed = false;
+
+          return vm.$nextTick();
+        })
+        .then(() => {
+          expect(vm.handleLoadCollapsedDiff).toHaveBeenCalled();
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+  });
 });
diff --git a/spec/models/concerns/awardable_spec.rb b/spec/models/concerns/awardable_spec.rb
index debc02fa51f..5713106418d 100644
--- a/spec/models/concerns/awardable_spec.rb
+++ b/spec/models/concerns/awardable_spec.rb
@@ -37,8 +37,8 @@ describe Awardable do
         create(:award_emoji, awardable: issue3, name: "star", user: award_emoji.user)
         create(:award_emoji, awardable: issue3, name: "star", user: award_emoji2.user)
 
-        expect(Issue.awarded(award_emoji.user)).to eq [issue, issue3]
-        expect(Issue.awarded(award_emoji2.user)).to eq [issue2, issue3]
+        expect(Issue.awarded(award_emoji.user)).to contain_exactly(issue, issue3)
+        expect(Issue.awarded(award_emoji2.user)).to contain_exactly(issue2, issue3)
       end
     end
 
diff --git a/spec/models/concerns/deployable_spec.rb b/spec/models/concerns/deployable_spec.rb
index ac79c75a55e..6951be903fe 100644
--- a/spec/models/concerns/deployable_spec.rb
+++ b/spec/models/concerns/deployable_spec.rb
@@ -49,5 +49,26 @@ describe Deployable do
         expect(environment).to be_nil
       end
     end
+
+    context 'when environment scope contains invalid character' do
+      let(:job) do
+        create(
+          :ci_build,
+          name: 'job:deploy-to-test-site',
+          environment: '$CI_JOB_NAME',
+          options: {
+            environment: {
+              name: '$CI_JOB_NAME',
+              url: 'http://staging.example.com/$CI_JOB_NAME',
+              on_stop: 'stop_review_app'
+            }
+          })
+      end
+
+      it 'does not create a deployment and environment record' do
+        expect(deployment).to be_nil
+        expect(environment).to be_nil
+      end
+    end
   end
 end
diff --git a/spec/serializers/environment_status_entity_spec.rb b/spec/serializers/environment_status_entity_spec.rb
index 52bd40ecb5e..5b3ebf0daa3 100644
--- a/spec/serializers/environment_status_entity_spec.rb
+++ b/spec/serializers/environment_status_entity_spec.rb
@@ -48,4 +48,40 @@ describe EnvironmentStatusEntity do
 
     it { is_expected.to include(:stop_url) }
   end
+
+  context 'when deployment has metrics' do
+    let(:prometheus_adapter) { double('prometheus_adapter', can_query?: true) }
+
+    let(:simple_metrics) do
+      {
+        success: true,
+        metrics: {},
+        last_update: 42
+      }
+    end
+
+    before do
+      project.add_maintainer(user)
+      allow(deployment).to receive(:prometheus_adapter).and_return(prometheus_adapter)
+      allow(prometheus_adapter).to receive(:query).with(:deployment, deployment).and_return(simple_metrics)
+      allow(entity).to receive(:deployment).and_return(deployment)
+    end
+
+    context 'when deployment succeeded' do
+      let(:deployment)    { create(:deployment, :succeed, :review_app) }
+
+      it 'returns metrics url' do
+        expect(subject[:metrics_url])
+          .to eq("/#{project.namespace.name}/#{project.name}/environments/#{environment.id}/deployments/#{deployment.iid}/metrics")
+      end
+    end
+
+    context 'when deployment is running' do
+      let(:deployment)    { create(:deployment, :running, :review_app) }
+
+      it 'does not return metrics url' do
+        expect(subject[:metrics_url]).to be_nil
+      end
+    end
+  end
 end
diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb
index 193148d403a..4d9c5aabbda 100644
--- a/spec/services/ci/create_pipeline_service_spec.rb
+++ b/spec/services/ci/create_pipeline_service_spec.rb
@@ -608,5 +608,53 @@ describe Ci::CreatePipelineService do
           .to eq variables_attributes.map(&:with_indifferent_access)
       end
     end
+
+    context 'when pipeline has a job with environment' do
+      let(:pipeline) { execute_service }
+
+      before do
+        stub_ci_pipeline_yaml_file(YAML.dump(config))
+      end
+
+      context 'when environment name is valid' do
+        let(:config) do
+          {
+            review_app: {
+              script: 'deploy',
+              environment: {
+                name: 'review/${CI_COMMIT_REF_NAME}',
+                url: 'http://${CI_COMMIT_REF_SLUG}-staging.example.com'
+              }
+            }
+          }
+        end
+
+        it 'has a job with environment' do
+          expect(pipeline.builds.count).to eq(1)
+          expect(pipeline.builds.first.persisted_environment.name).to eq('review/master')
+          expect(pipeline.builds.first.deployment).to be_created
+        end
+      end
+
+      context 'when environment name is invalid' do
+        let(:config) do
+          {
+            'job:deploy-to-test-site': {
+              script: 'deploy',
+              environment: {
+                name: '${CI_JOB_NAME}',
+                url: 'https://$APP_URL'
+              }
+            }
+          }
+        end
+
+        it 'has a job without environment' do
+          expect(pipeline.builds.count).to eq(1)
+          expect(pipeline.builds.first.persisted_environment).to be_nil
+          expect(pipeline.builds.first.deployment).to be_nil
+        end
+      end
+    end
   end
 end