diff options
author | Steve Azzopardi <sazzopardi@gitlab.com> | 2018-11-15 14:35:43 +0000 |
---|---|---|
committer | Steve Azzopardi <sazzopardi@gitlab.com> | 2018-11-15 14:35:43 +0000 |
commit | 91f80cda8e9e552d2d490ea44a1b4f9097922899 (patch) | |
tree | e1311f8003519e94d470db20f46ff5d092f09183 | |
parent | bf58952b523012017d483f71e784f5365a4dcc17 (diff) | |
parent | d03c99a39db8db1e6aa86a3e4d77db95ae8b32e5 (diff) | |
download | gitlab-ce-91f80cda8e9e552d2d490ea44a1b4f9097922899.tar.gz |
Merge branch '11-5-stable-prepare-rc10' into '11-5-stable'
Prepare 11.5 RC10 release
See merge request gitlab-org/gitlab-ce!23116
-rw-r--r-- | app/assets/javascripts/diffs/components/diff_file.vue | 18 | ||||
-rw-r--r-- | app/assets/stylesheets/pages/notes.scss | 27 | ||||
-rw-r--r-- | app/models/concerns/deployable.rb | 4 | ||||
-rw-r--r-- | app/models/deployment.rb | 6 | ||||
-rw-r--r-- | app/serializers/environment_status_entity.rb | 6 | ||||
-rw-r--r-- | changelogs/unreleased/fix-deployment-metrics-in-mr-widget.yml | 6 | ||||
-rw-r--r-- | changelogs/unreleased/ignore-environment-validation-failure.yml | 5 | ||||
-rw-r--r-- | spec/javascripts/diffs/components/diff_file_spec.js | 22 | ||||
-rw-r--r-- | spec/models/concerns/awardable_spec.rb | 4 | ||||
-rw-r--r-- | spec/models/concerns/deployable_spec.rb | 21 | ||||
-rw-r--r-- | spec/serializers/environment_status_entity_spec.rb | 36 | ||||
-rw-r--r-- | spec/services/ci/create_pipeline_service_spec.rb | 48 |
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 |