From d0b7bc79fa0cd663ba5a789b14dc49e1b29bb3d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Wed, 14 Nov 2018 11:11:27 +0000 Subject: Merge branch 'fix-deployment-metrics-in-mr-widget' into 'master' Avoid returning deployment metrics url to MR widget when the deployment is not successful Closes #53870 See merge request gitlab-org/gitlab-ce!23010 (cherry picked from commit 7674a8f477c90c1c8c9a969e7d80ea1ec9e72cd9) e270fcc8 Fix deployment metrics in MR widget 15431054 Add spec for deployment metrics 2d6570f0 Do not remove the existing permission check 09e693c6 Add changelog d8c24ac1 Remove unrelated changes --- app/models/deployment.rb | 6 ++-- app/serializers/environment_status_entity.rb | 6 +++- .../fix-deployment-metrics-in-mr-widget.yml | 6 ++++ spec/serializers/environment_status_entity_spec.rb | 36 ++++++++++++++++++++++ 4 files changed, 50 insertions(+), 4 deletions(-) create mode 100644 changelogs/unreleased/fix-deployment-metrics-in-mr-widget.yml 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/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 -- cgit v1.2.3 From 7c905c669ccb99a70f734bc13003646449007e42 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Wed, 14 Nov 2018 15:41:19 +0000 Subject: Merge branch 'mr-expand-all-collapsed-files' into 'master' Fix collapsed files not fully fully expanding Closes #53866 See merge request gitlab-org/gitlab-ce!23019 (cherry picked from commit 63b4b4b2688fa4f068772026536b2250bce39070) 8e265bc3 Fix collapsed files not fully fully expanding --- .../javascripts/diffs/components/diff_file.vue | 18 +++++++++++++++--- .../javascripts/diffs/components/diff_file_spec.js | 22 ++++++++++++++++++++++ 2 files changed, 37 insertions(+), 3 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/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); + }); + }); }); -- cgit v1.2.3 From 09f8ddad41587636cc5ce179586aa6049f3f8f4c Mon Sep 17 00:00:00 2001 From: Fatih Acet Date: Wed, 14 Nov 2018 23:56:10 +0000 Subject: Merge branch '53882-extra-container-diff-version' into 'master' Fix version system note Closes #53882 See merge request gitlab-org/gitlab-ce!23030 (cherry picked from commit d645df8220669b7ccb889349d412bf527d621fc4) f25e6aa2 Fix version system note d0f8e279 Remove box on diff tab system note 0c4170b8 Add back margin on discussion system notes 5e66292d Tweak vertical line to line up with system notes and comment avatars --- app/assets/stylesheets/pages/notes.scss | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) 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 { -- cgit v1.2.3 From 781257abc5653ed4798a73c0d36859dc1ed4aecb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Thu, 15 Nov 2018 11:19:04 +0000 Subject: Merge branch 'ignore-environment-validation-failure' into 'master' Ignore environment validation failure See merge request gitlab-org/gitlab-ce!23100 (cherry picked from commit 0f25d2b33fbee7161f0ecf26a6d853533808beec) ea695ab7 Ignore environment validation failure a2a2a8f0 Add changelog 00842f95 User persisted? --- app/models/concerns/deployable.rb | 4 ++ .../ignore-environment-validation-failure.yml | 5 +++ spec/models/concerns/deployable_spec.rb | 21 ++++++++++ spec/services/ci/create_pipeline_service_spec.rb | 48 ++++++++++++++++++++++ 4 files changed, 78 insertions(+) create mode 100644 changelogs/unreleased/ignore-environment-validation-failure.yml 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/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/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/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 -- cgit v1.2.3 From d03c99a39db8db1e6aa86a3e4d77db95ae8b32e5 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 15 Nov 2018 08:51:40 +0000 Subject: Merge branch '53491-fix-again' into 'master' Don't care about order when getting awarded issues Closes #53491 See merge request gitlab-org/gitlab-ce!23101 (cherry picked from commit f3c44f640a0644590485d61d36ad222b343ec139) 8ae9b248 Don't care about order when getting awarded issues --- spec/models/concerns/awardable_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 -- cgit v1.2.3