From 91f8e734fee1f9e7a16573f7185c48f442e3bb5e Mon Sep 17 00:00:00 2001 From: Alessio Caiazza Date: Mon, 25 Sep 2017 18:54:08 +0200 Subject: Add CI build trace sections extractor --- app/models/ci/build.rb | 22 +++++ app/models/ci/build_trace_section.rb | 11 +++ app/models/ci/build_trace_section_name.rb | 11 +++ app/models/project.rb | 1 + app/workers/build_finished_worker.rb | 1 + app/workers/build_trace_sections_worker.rb | 8 ++ .../unreleased/37970-ci-sections-tracking.yml | 5 ++ ...0170926102201_create_ci_build_trace_sections.rb | 19 +++++ ...build_foreign_key_to_ci_build_trace_sections.rb | 15 ++++ ...29123651_create_ci_build_trace_section_names.rb | 19 +++++ ..._name_foreign_key_to_ci_build_trace_sections.rb | 15 ++++ db/schema.rb | 24 ++++++ lib/gitlab/ci/ansi2html.rb | 2 +- lib/gitlab/ci/trace.rb | 6 ++ lib/gitlab/ci/trace/section_parser.rb | 97 ++++++++++++++++++++++ lib/gitlab/ci/trace/stream.rb | 17 ++++ lib/gitlab/regex.rb | 4 + spec/factories/ci/build_trace_section_names.rb | 6 ++ spec/fixtures/trace/trace_with_sections | 15 ++++ spec/lib/gitlab/ci/trace/section_parser_spec.rb | 87 +++++++++++++++++++ spec/lib/gitlab/ci/trace_spec.rb | 87 +++++++++++++++++++ spec/lib/gitlab/import_export/all_models.yml | 1 + spec/models/ci/build_spec.rb | 23 +++++ spec/models/ci/build_trace_section_name_spec.rb | 12 +++ spec/models/ci/build_trace_section_spec.rb | 11 +++ spec/models/project_spec.rb | 1 + spec/services/ci/retry_build_service_spec.rb | 2 +- spec/workers/build_finished_worker_spec.rb | 2 + spec/workers/build_trace_sections_worker_spec.rb | 23 +++++ 29 files changed, 545 insertions(+), 2 deletions(-) create mode 100644 app/models/ci/build_trace_section.rb create mode 100644 app/models/ci/build_trace_section_name.rb create mode 100644 app/workers/build_trace_sections_worker.rb create mode 100644 changelogs/unreleased/37970-ci-sections-tracking.yml create mode 100644 db/migrate/20170926102201_create_ci_build_trace_sections.rb create mode 100644 db/migrate/20170926134436_add_build_foreign_key_to_ci_build_trace_sections.rb create mode 100644 db/migrate/20170929123651_create_ci_build_trace_section_names.rb create mode 100644 db/migrate/20170929123915_add_name_foreign_key_to_ci_build_trace_sections.rb create mode 100644 lib/gitlab/ci/trace/section_parser.rb create mode 100644 spec/factories/ci/build_trace_section_names.rb create mode 100644 spec/fixtures/trace/trace_with_sections create mode 100644 spec/lib/gitlab/ci/trace/section_parser_spec.rb create mode 100644 spec/models/ci/build_trace_section_name_spec.rb create mode 100644 spec/models/ci/build_trace_section_spec.rb create mode 100644 spec/workers/build_trace_sections_worker_spec.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index dd315866e60..72e646210a8 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -11,6 +11,7 @@ module Ci has_many :deployments, as: :deployable has_one :last_deployment, -> { order('deployments.id DESC') }, as: :deployable, class_name: 'Deployment' + has_many :trace_sections, class_name: 'Ci::BuildTraceSection' # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment @@ -265,6 +266,27 @@ module Ci update_attributes(coverage: coverage) if coverage.present? end + def parse_trace_sections! + return false unless trace_sections.empty? + + sections = trace.extract_sections.map do |attr| + name = attr.delete(:name) + name_record = begin + project.build_trace_section_names.find_or_create_by!(name: name) + rescue ActiveRecord::RecordInvalid + project.build_trace_section_names.find_by!(name: name) + end + + attr.merge( + build_id: self.id, + project_id: self.project_id, + section_name_id: name_record.id) + end + + Gitlab::Database.bulk_insert(Ci::BuildTraceSection.table_name, sections) + true + end + def trace Gitlab::Ci::Trace.new(self) end diff --git a/app/models/ci/build_trace_section.rb b/app/models/ci/build_trace_section.rb new file mode 100644 index 00000000000..ccdb95546c8 --- /dev/null +++ b/app/models/ci/build_trace_section.rb @@ -0,0 +1,11 @@ +module Ci + class BuildTraceSection < ActiveRecord::Base + extend Gitlab::Ci::Model + + belongs_to :build, class_name: 'Ci::Build' + belongs_to :project + belongs_to :section_name, class_name: 'Ci::BuildTraceSectionName' + + validates :section_name, :build, :project, presence: true, allow_blank: false + end +end diff --git a/app/models/ci/build_trace_section_name.rb b/app/models/ci/build_trace_section_name.rb new file mode 100644 index 00000000000..0fdcb1ea329 --- /dev/null +++ b/app/models/ci/build_trace_section_name.rb @@ -0,0 +1,11 @@ +module Ci + class BuildTraceSectionName < ActiveRecord::Base + extend Gitlab::Ci::Model + + belongs_to :project + has_many :trace_sections, class_name: 'Ci::BuildTraceSection', foreign_key: :section_name_id + + validates :name, :project, presence: true, allow_blank: false + validates :name, uniqueness: { scope: :project_id } + end +end diff --git a/app/models/project.rb b/app/models/project.rb index 59b5a5b3cd7..4d4d028dd7e 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -179,6 +179,7 @@ class Project < ActiveRecord::Base # bulk that doesn't involve loading the rows into memory. As a result we're # still using `dependent: :destroy` here. has_many :builds, class_name: 'Ci::Build', dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_many :build_trace_section_names, class_name: 'Ci::BuildTraceSectionName' has_many :runner_projects, class_name: 'Ci::RunnerProject' has_many :runners, through: :runner_projects, source: :runner, class_name: 'Ci::Runner' has_many :variables, class_name: 'Ci::Variable' diff --git a/app/workers/build_finished_worker.rb b/app/workers/build_finished_worker.rb index e2a1b3dcc41..52e7d346e74 100644 --- a/app/workers/build_finished_worker.rb +++ b/app/workers/build_finished_worker.rb @@ -6,6 +6,7 @@ class BuildFinishedWorker def perform(build_id) Ci::Build.find_by(id: build_id).try do |build| + BuildTraceSectionsWorker.perform_async(build.id) BuildCoverageWorker.new.perform(build.id) BuildHooksWorker.new.perform(build.id) end diff --git a/app/workers/build_trace_sections_worker.rb b/app/workers/build_trace_sections_worker.rb new file mode 100644 index 00000000000..8c57e8f767b --- /dev/null +++ b/app/workers/build_trace_sections_worker.rb @@ -0,0 +1,8 @@ +class BuildTraceSectionsWorker + include Sidekiq::Worker + include PipelineQueue + + def perform(build_id) + Ci::Build.find_by(id: build_id)&.parse_trace_sections! + end +end diff --git a/changelogs/unreleased/37970-ci-sections-tracking.yml b/changelogs/unreleased/37970-ci-sections-tracking.yml new file mode 100644 index 00000000000..a9011b22c6c --- /dev/null +++ b/changelogs/unreleased/37970-ci-sections-tracking.yml @@ -0,0 +1,5 @@ +--- +title: Parse and store gitlab-runner timestamped section markers +merge_request: 14551 +author: +type: added diff --git a/db/migrate/20170926102201_create_ci_build_trace_sections.rb b/db/migrate/20170926102201_create_ci_build_trace_sections.rb new file mode 100644 index 00000000000..ad11b31b75d --- /dev/null +++ b/db/migrate/20170926102201_create_ci_build_trace_sections.rb @@ -0,0 +1,19 @@ +class CreateCiBuildTraceSections < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + create_table :ci_build_trace_sections do |t| + t.datetime_with_timezone :date_start, null: false + t.datetime_with_timezone :date_end, null: false + t.integer :byte_start, limit: 8, null: false + t.integer :byte_end, limit: 8, null: false + t.references :project, null: false, index: true, foreign_key: { on_delete: :cascade } + t.integer :build_id, null: false + t.integer :section_name_id, null: false + end + + add_index :ci_build_trace_sections, [:build_id, :section_name_id], unique: true + end +end diff --git a/db/migrate/20170926134436_add_build_foreign_key_to_ci_build_trace_sections.rb b/db/migrate/20170926134436_add_build_foreign_key_to_ci_build_trace_sections.rb new file mode 100644 index 00000000000..d279463eb4b --- /dev/null +++ b/db/migrate/20170926134436_add_build_foreign_key_to_ci_build_trace_sections.rb @@ -0,0 +1,15 @@ +class AddBuildForeignKeyToCiBuildTraceSections < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_foreign_key(:ci_build_trace_sections, :ci_builds, column: :build_id) + end + + def down + remove_foreign_key(:ci_build_trace_sections, column: :build_id) + end +end diff --git a/db/migrate/20170929123651_create_ci_build_trace_section_names.rb b/db/migrate/20170929123651_create_ci_build_trace_section_names.rb new file mode 100644 index 00000000000..88f3e60699a --- /dev/null +++ b/db/migrate/20170929123651_create_ci_build_trace_section_names.rb @@ -0,0 +1,19 @@ +class CreateCiBuildTraceSectionNames < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + create_table :ci_build_trace_section_names do |t| + t.references :project, null: false, foreign_key: { on_delete: :cascade } + t.string :name, null: false + end + + add_index :ci_build_trace_section_names, [:project_id, :name], unique: true + end + + def down + remove_foreign_key :ci_build_trace_section_names, column: :project_id + drop_table :ci_build_trace_section_names + end +end diff --git a/db/migrate/20170929123915_add_name_foreign_key_to_ci_build_trace_sections.rb b/db/migrate/20170929123915_add_name_foreign_key_to_ci_build_trace_sections.rb new file mode 100644 index 00000000000..08422885a98 --- /dev/null +++ b/db/migrate/20170929123915_add_name_foreign_key_to_ci_build_trace_sections.rb @@ -0,0 +1,15 @@ +class AddNameForeignKeyToCiBuildTraceSections < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_foreign_key(:ci_build_trace_sections, :ci_build_trace_section_names, column: :section_name_id) + end + + def down + remove_foreign_key(:ci_build_trace_sections, column: :section_name_id) + end +end diff --git a/db/schema.rb b/db/schema.rb index fa1aad257db..4f2424ff2ab 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -207,6 +207,26 @@ ActiveRecord::Schema.define(version: 20171004121444) do add_index "chat_teams", ["namespace_id"], name: "index_chat_teams_on_namespace_id", unique: true, using: :btree + create_table "ci_build_trace_section_names", force: :cascade do |t| + t.integer "project_id", null: false + t.string "name", null: false + end + + add_index "ci_build_trace_section_names", ["project_id", "name"], name: "index_ci_build_trace_section_names_on_project_id_and_name", unique: true, using: :btree + + create_table "ci_build_trace_sections", force: :cascade do |t| + t.datetime_with_timezone "date_start", null: false + t.datetime_with_timezone "date_end", null: false + t.integer "byte_start", limit: 8, null: false + t.integer "byte_end", limit: 8, null: false + t.integer "project_id", null: false + t.integer "build_id", null: false + t.integer "section_name_id", null: false + end + + add_index "ci_build_trace_sections", ["build_id", "section_name_id"], name: "index_ci_build_trace_sections_on_build_id_and_section_name_id", unique: true, using: :btree + add_index "ci_build_trace_sections", ["project_id"], name: "index_ci_build_trace_sections_on_project_id", using: :btree + create_table "ci_builds", force: :cascade do |t| t.string "status" t.datetime "finished_at" @@ -1697,6 +1717,10 @@ ActiveRecord::Schema.define(version: 20171004121444) do add_foreign_key "boards", "projects", name: "fk_f15266b5f9", on_delete: :cascade add_foreign_key "chat_teams", "namespaces", on_delete: :cascade + add_foreign_key "ci_build_trace_section_names", "projects", on_delete: :cascade + add_foreign_key "ci_build_trace_sections", "ci_build_trace_section_names", column: "section_name_id", name: "fk_264e112c66", on_delete: :cascade + add_foreign_key "ci_build_trace_sections", "ci_builds", column: "build_id", name: "fk_4ebe41f502", on_delete: :cascade + add_foreign_key "ci_build_trace_sections", "projects", on_delete: :cascade add_foreign_key "ci_builds", "ci_pipelines", column: "auto_canceled_by_id", name: "fk_a2141b1522", on_delete: :nullify add_foreign_key "ci_builds", "ci_stages", column: "stage_id", name: "fk_3a9eaa254d", on_delete: :cascade add_foreign_key "ci_builds", "projects", name: "fk_befce0568a", on_delete: :cascade diff --git a/lib/gitlab/ci/ansi2html.rb b/lib/gitlab/ci/ansi2html.rb index 088adbdd267..72b75791bbb 100644 --- a/lib/gitlab/ci/ansi2html.rb +++ b/lib/gitlab/ci/ansi2html.rb @@ -155,7 +155,7 @@ module Gitlab stream.each_line do |line| s = StringScanner.new(line) until s.eos? - if s.scan(/section_((?:start)|(?:end)):(\d+):([^\r]+)\r\033\[0K/) + if s.scan(Gitlab::Regex.build_trace_section_regex) handle_section(s) elsif s.scan(/\e([@-_])(.*?)([@-~])/) handle_sequence(s) diff --git a/lib/gitlab/ci/trace.rb b/lib/gitlab/ci/trace.rb index 5b835bb669a..baf55b1fa07 100644 --- a/lib/gitlab/ci/trace.rb +++ b/lib/gitlab/ci/trace.rb @@ -27,6 +27,12 @@ module Gitlab end end + def extract_sections + read do |stream| + stream.extract_sections + end + end + def set(data) write do |stream| data = job.hide_secrets(data) diff --git a/lib/gitlab/ci/trace/section_parser.rb b/lib/gitlab/ci/trace/section_parser.rb new file mode 100644 index 00000000000..9bb0166c9e3 --- /dev/null +++ b/lib/gitlab/ci/trace/section_parser.rb @@ -0,0 +1,97 @@ +module Gitlab + module Ci + class Trace + class SectionParser + def initialize(lines) + @lines = lines + end + + def parse! + @markers = {} + + @lines.each do |line, pos| + parse_line(line, pos) + end + end + + def sections + sanitize_markers.map do |name, markers| + start_, end_ = markers + + { + name: name, + byte_start: start_[:marker], + byte_end: end_[:marker], + date_start: start_[:timestamp], + date_end: end_[:timestamp] + } + end + end + + private + + def parse_line(line, line_start_position) + s = StringScanner.new(line) + until s.eos? + find_next_marker(s) do |scanner| + marker_begins_at = line_start_position + scanner.pointer + + if scanner.scan(Gitlab::Regex.build_trace_section_regex) + marker_ends_at = line_start_position + scanner.pointer + handle_line(scanner[1], scanner[2].to_i, scanner[3], marker_begins_at, marker_ends_at) + true + else + false + end + end + end + end + + def sanitize_markers + @markers.select do |_, markers| + markers.size == 2 && markers[0][:action] == :start && markers[1][:action] == :end + end + end + + def handle_line(action, time, name, marker_start, marker_end) + action = action.to_sym + timestamp = Time.at(time).utc + marker = if action == :start + marker_end + else + marker_start + end + + @markers[name] ||= [] + @markers[name] << { + name: name, + action: action, + timestamp: timestamp, + marker: marker + } + end + + def beginning_of_section_regex + @beginning_of_section_regex ||= /section_/.freeze + end + + def find_next_marker(s) + beginning_of_section_len = 8 + maybe_marker = s.exist?(beginning_of_section_regex) + + if maybe_marker.nil? + s.terminate + else + # repositioning at the beginning of the match + s.pos += maybe_marker - beginning_of_section_len + if block_given? + good_marker = yield(s) + # if not a good marker: Consuming the matched beginning_of_section_regex + s.pos += beginning_of_section_len unless good_marker + end + end + end + end + end + end +end diff --git a/lib/gitlab/ci/trace/stream.rb b/lib/gitlab/ci/trace/stream.rb index ab3408f48d6..d52194f688b 100644 --- a/lib/gitlab/ci/trace/stream.rb +++ b/lib/gitlab/ci/trace/stream.rb @@ -90,8 +90,25 @@ module Gitlab # so we just silently ignore error for now end + def extract_sections + return [] unless valid? + + lines = to_enum(:each_line_with_pos) + parser = SectionParser.new(lines) + + parser.parse! + parser.sections + end + private + def each_line_with_pos + stream.seek(0, IO::SEEK_SET) + stream.each_line do |line| + yield [line, stream.pos - line.bytesize] + end + end + def read_last_lines(limit) to_enum(:reverse_line).first(limit).reverse.join end diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index 58f6245579a..bd677ec4bf3 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -65,5 +65,9 @@ module Gitlab "can contain only lowercase letters, digits, and '-'. " \ "Must start with a letter, and cannot end with '-'" end + + def build_trace_section_regex + @build_trace_section_regexp ||= /section_((?:start)|(?:end)):(\d+):([^\r]+)\r\033\[0K/.freeze + end end end diff --git a/spec/factories/ci/build_trace_section_names.rb b/spec/factories/ci/build_trace_section_names.rb new file mode 100644 index 00000000000..1c16225f0e5 --- /dev/null +++ b/spec/factories/ci/build_trace_section_names.rb @@ -0,0 +1,6 @@ +FactoryGirl.define do + factory :ci_build_trace_section_name, class: Ci::BuildTraceSectionName do + sequence(:name) { |n| "section_#{n}" } + project factory: :project + end +end diff --git a/spec/fixtures/trace/trace_with_sections b/spec/fixtures/trace/trace_with_sections new file mode 100644 index 00000000000..21dff3928c3 --- /dev/null +++ b/spec/fixtures/trace/trace_with_sections @@ -0,0 +1,15 @@ +Running with gitlab-runner dev (HEAD) + on kitsune minikube (a21b584f) +WARNING: Namespace is empty, therefore assuming 'default'. +Using Kubernetes namespace: default +Using Kubernetes executor with image alpine:3.4 ... +section_start:1506004954:prepare_script Waiting for pod default/runner-a21b584f-project-1208199-concurrent-0sg03f to be running, status is Pending +Running on runner-a21b584f-project-1208199-concurrent-0sg03f via kitsune.local... +section_end:1506004957:prepare_script section_start:1506004957:get_sources Cloning repository... +Cloning into '/nolith/ci-tests'... +Checking out dddd7a6e as master... +Skipping Git submodules setup +section_end:1506004958:get_sources section_start:1506004958:restore_cache section_end:1506004958:restore_cache section_start:1506004958:download_artifacts section_end:1506004958:download_artifacts section_start:1506004958:build_script $ whoami +root +section_end:1506004959:build_script section_start:1506004959:after_script section_end:1506004959:after_script section_start:1506004959:archive_cache section_end:1506004959:archive_cache section_start:1506004959:upload_artifacts section_end:1506004959:upload_artifacts Job succeeded + diff --git a/spec/lib/gitlab/ci/trace/section_parser_spec.rb b/spec/lib/gitlab/ci/trace/section_parser_spec.rb new file mode 100644 index 00000000000..ca53ff87c6f --- /dev/null +++ b/spec/lib/gitlab/ci/trace/section_parser_spec.rb @@ -0,0 +1,87 @@ +require 'spec_helper' + +describe Gitlab::Ci::Trace::SectionParser do + def lines_with_pos(text) + pos = 0 + StringIO.new(text).each_line do |line| + yield line, pos + pos += line.bytesize + 1 # newline + end + end + + def build_lines(text) + to_enum(:lines_with_pos, text) + end + + def section(name, start, duration, text) + end_ = start + duration + "section_start:#{start.to_i}:#{name}\r\033[0K#{text}section_end:#{end_.to_i}:#{name}\r\033[0K" + end + + let(:lines) { build_lines('') } + subject { described_class.new(lines) } + + describe '#sections' do + before do + subject.parse! + end + + context 'empty trace' do + let(:lines) { build_lines('') } + + it { expect(subject.sections).to be_empty } + end + + context 'with a sectionless trace' do + let(:lines) { build_lines("line 1\nline 2\n") } + + it { expect(subject.sections).to be_empty } + end + + context 'with trace markers' do + let(:start_time) { Time.new(2017, 10, 5).utc } + let(:section_b_duration) { 1.second } + let(:section_a) { section('a', start_time, 0, 'a line') } + let(:section_b) { section('b', start_time, section_b_duration, "another line\n") } + let(:lines) { build_lines(section_a + section_b) } + + it { expect(subject.sections.size).to eq(2) } + it { expect(subject.sections[1][:name]).to eq('b') } + it { expect(subject.sections[1][:date_start]).to eq(start_time) } + it { expect(subject.sections[1][:date_end]).to eq(start_time + section_b_duration) } + end + end + + describe '#parse!' do + context 'multiple "section_" but no complete markers' do + let(:lines) { build_lines('section_section_section_') } + + it 'must find 3 possible section start but no complete sections' do + expect(subject).to receive(:find_next_marker).exactly(3).times.and_call_original + + subject.parse! + + expect(subject.sections).to be_empty + end + end + + context 'trace with UTF-8 chars' do + let(:line) { 'GitLab ❤️ 狸 (tanukis)\n' } + let(:trace) { section('test_section', Time.new(2017, 10, 5).utc, 3.seconds, line) } + let(:lines) { build_lines(trace) } + + it 'must handle correctly byte positioning' do + expect(subject).to receive(:find_next_marker).exactly(2).times.and_call_original + + subject.parse! + + sections = subject.sections + + expect(sections.size).to eq(1) + s = sections[0] + len = s[:byte_end] - s[:byte_start] + expect(trace.byteslice(s[:byte_start], len)).to eq(line) + end + end + end +end diff --git a/spec/lib/gitlab/ci/trace_spec.rb b/spec/lib/gitlab/ci/trace_spec.rb index 9cb0b62590a..3546532b9b4 100644 --- a/spec/lib/gitlab/ci/trace_spec.rb +++ b/spec/lib/gitlab/ci/trace_spec.rb @@ -61,6 +61,93 @@ describe Gitlab::Ci::Trace do end end + describe '#extract_sections' do + let(:log) { 'No sections' } + let(:sections) { trace.extract_sections } + + before do + trace.set(log) + end + + context 'no sections' do + it 'returs []' do + expect(trace.extract_sections).to eq([]) + end + end + + context 'multiple sections available' do + let(:log) { File.read(expand_fixture_path('trace/trace_with_sections')) } + let(:sections_data) do + [ + { name: 'prepare_script', lines: 2, duration: 3.seconds }, + { name: 'get_sources', lines: 4, duration: 1.second }, + { name: 'restore_cache', lines: 0, duration: 0.seconds }, + { name: 'download_artifacts', lines: 0, duration: 0.seconds }, + { name: 'build_script', lines: 2, duration: 1.second }, + { name: 'after_script', lines: 0, duration: 0.seconds }, + { name: 'archive_cache', lines: 0, duration: 0.seconds }, + { name: 'upload_artifacts', lines: 0, duration: 0.seconds } + ] + end + + it "returns valid sections" do + expect(sections).not_to be_empty + expect(sections.size).to eq(sections_data.size), + "expected #{sections_data.size} sections, got #{sections.size}" + + buff = StringIO.new(log) + sections.each_with_index do |s, i| + expected = sections_data[i] + + expect(s[:name]).to eq(expected[:name]) + expect(s[:date_end] - s[:date_start]).to eq(expected[:duration]) + + buff.seek(s[:byte_start], IO::SEEK_SET) + length = s[:byte_end] - s[:byte_start] + lines = buff.read(length).count("\n") + expect(lines).to eq(expected[:lines]) + end + end + end + + context 'logs contains "section_start"' do + let(:log) { "section_start:1506417476:a_section\r\033[0Klooks like a section_start:invalid\nsection_end:1506417477:a_section\r\033[0K"} + + it "returns only one section" do + expect(sections).not_to be_empty + expect(sections.size).to eq(1) + + section = sections[0] + expect(section[:name]).to eq('a_section') + expect(section[:byte_start]).not_to eq(section[:byte_end]), "got an empty section" + end + end + + context 'missing section_end' do + let(:log) { "section_start:1506417476:a_section\r\033[0KSome logs\nNo section_end\n"} + + it "returns no sections" do + expect(sections).to be_empty + end + end + + context 'missing section_start' do + let(:log) { "Some logs\nNo section_start\nsection_end:1506417476:a_section\r\033[0K"} + + it "returns no sections" do + expect(sections).to be_empty + end + end + + context 'inverted section_start section_end' do + let(:log) { "section_end:1506417476:a_section\r\033[0Klooks like a section_start:invalid\nsection_start:1506417477:a_section\r\033[0K"} + + it "returns no sections" do + expect(sections).to be_empty + end + end + end + describe '#set' do before do trace.set("12") diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 3fb8edeb701..195997e3501 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -266,6 +266,7 @@ project: - container_repositories - uploads - members_and_requesters +- build_trace_section_names award_emoji: - awardable - user diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 451968c7342..7fe12f92a2a 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -18,6 +18,7 @@ describe Ci::Build do it { is_expected.to belong_to(:trigger_request) } it { is_expected.to belong_to(:erased_by) } it { is_expected.to have_many(:deployments) } + it { is_expected.to have_many(:trace_sections)} it { is_expected.to validate_presence_of(:ref) } it { is_expected.to respond_to(:has_trace?) } it { is_expected.to respond_to(:trace) } @@ -320,6 +321,28 @@ describe Ci::Build do end end + describe '#parse_trace_sections!' do + context "when the build trace has sections markers," do + before do + build.trace.set(File.read(expand_fixture_path('trace/trace_with_sections'))) + end + + it "saves the correct extracted sections" do + expect(build.trace_sections).to be_empty + expect(build.parse_trace_sections!).to be(true) + expect(build.trace_sections).not_to be_empty + end + + it "fails if trace_sections isn't empty" do + expect(build.parse_trace_sections!).to be(true) + expect(build.trace_sections).not_to be_empty + + expect(build.parse_trace_sections!).to be(false) + expect(build.trace_sections).not_to be_empty + end + end + end + describe '#trace' do subject { build.trace } diff --git a/spec/models/ci/build_trace_section_name_spec.rb b/spec/models/ci/build_trace_section_name_spec.rb new file mode 100644 index 00000000000..386ee6880cb --- /dev/null +++ b/spec/models/ci/build_trace_section_name_spec.rb @@ -0,0 +1,12 @@ +require 'spec_helper' + +describe Ci::BuildTraceSectionName, model: true do + subject { build(:ci_build_trace_section_name) } + + it { is_expected.to belong_to(:project) } + it { is_expected.to have_many(:trace_sections)} + + it { is_expected.to validate_presence_of(:project) } + it { is_expected.to validate_presence_of(:name) } + it { is_expected.to validate_uniqueness_of(:name).scoped_to(:project_id) } +end diff --git a/spec/models/ci/build_trace_section_spec.rb b/spec/models/ci/build_trace_section_spec.rb new file mode 100644 index 00000000000..541a9a36fb8 --- /dev/null +++ b/spec/models/ci/build_trace_section_spec.rb @@ -0,0 +1,11 @@ +require 'spec_helper' + +describe Ci::BuildTraceSection, model: true do + it { is_expected.to belong_to(:build)} + it { is_expected.to belong_to(:project)} + it { is_expected.to belong_to(:section_name)} + + it { is_expected.to validate_presence_of(:section_name) } + it { is_expected.to validate_presence_of(:build) } + it { is_expected.to validate_presence_of(:project) } +end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 176bb568cbe..1c04cb8cb5f 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -57,6 +57,7 @@ describe Project do it { is_expected.to have_many(:commit_statuses) } it { is_expected.to have_many(:pipelines) } it { is_expected.to have_many(:builds) } + it { is_expected.to have_many(:build_trace_section_names)} it { is_expected.to have_many(:runner_projects) } it { is_expected.to have_many(:runners) } it { is_expected.to have_many(:active_runners) } diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index fbb3213f42b..9db3568abee 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -20,7 +20,7 @@ describe Ci::RetryBuildService do erased_at auto_canceled_by].freeze IGNORE_ACCESSORS = - %i[type lock_version target_url base_tags + %i[type lock_version target_url base_tags trace_sections commit_id deployments erased_by_id last_deployment project_id runner_id tag_taggings taggings tags trigger_request_id user_id auto_canceled_by_id retried failure_reason].freeze diff --git a/spec/workers/build_finished_worker_spec.rb b/spec/workers/build_finished_worker_spec.rb index 8cc3f37ebe8..1a7ffd5cdbf 100644 --- a/spec/workers/build_finished_worker_spec.rb +++ b/spec/workers/build_finished_worker_spec.rb @@ -11,6 +11,8 @@ describe BuildFinishedWorker do expect(BuildHooksWorker) .to receive(:new).ordered.and_call_original + expect(BuildTraceSectionsWorker) + .to receive(:perform_async) expect_any_instance_of(BuildCoverageWorker) .to receive(:perform) expect_any_instance_of(BuildHooksWorker) diff --git a/spec/workers/build_trace_sections_worker_spec.rb b/spec/workers/build_trace_sections_worker_spec.rb new file mode 100644 index 00000000000..45243f45547 --- /dev/null +++ b/spec/workers/build_trace_sections_worker_spec.rb @@ -0,0 +1,23 @@ +require 'spec_helper' + +describe BuildTraceSectionsWorker do + describe '#perform' do + context 'when build exists' do + let!(:build) { create(:ci_build) } + + it 'updates trace sections' do + expect_any_instance_of(Ci::Build) + .to receive(:parse_trace_sections!) + + described_class.new.perform(build.id) + end + end + + context 'when build does not exist' do + it 'does not raise exception' do + expect { described_class.new.perform(123) } + .not_to raise_error + end + end + end +end -- cgit v1.2.1 From ea023138bf5116a729e5accd5f81d4e586af6b02 Mon Sep 17 00:00:00 2001 From: Alessio Caiazza Date: Fri, 6 Oct 2017 11:05:23 +0200 Subject: Repack migration correctly --- .../20170926102201_create_ci_build_trace_sections.rb | 19 ------------------- ...dd_build_foreign_key_to_ci_build_trace_sections.rb | 15 --------------- ...70929123651_create_ci_build_trace_section_names.rb | 19 ------------------- ...add_name_foreign_key_to_ci_build_trace_sections.rb | 15 --------------- .../20171006090001_create_ci_build_trace_sections.rb | 19 +++++++++++++++++++ ...dd_build_foreign_key_to_ci_build_trace_sections.rb | 15 +++++++++++++++ ...71006090100_create_ci_build_trace_section_names.rb | 19 +++++++++++++++++++ ...add_name_foreign_key_to_ci_build_trace_sections.rb | 15 +++++++++++++++ db/schema.rb | 4 ++-- 9 files changed, 70 insertions(+), 70 deletions(-) delete mode 100644 db/migrate/20170926102201_create_ci_build_trace_sections.rb delete mode 100644 db/migrate/20170926134436_add_build_foreign_key_to_ci_build_trace_sections.rb delete mode 100644 db/migrate/20170929123651_create_ci_build_trace_section_names.rb delete mode 100644 db/migrate/20170929123915_add_name_foreign_key_to_ci_build_trace_sections.rb create mode 100644 db/migrate/20171006090001_create_ci_build_trace_sections.rb create mode 100644 db/migrate/20171006090010_add_build_foreign_key_to_ci_build_trace_sections.rb create mode 100644 db/migrate/20171006090100_create_ci_build_trace_section_names.rb create mode 100644 db/migrate/20171006091000_add_name_foreign_key_to_ci_build_trace_sections.rb diff --git a/db/migrate/20170926102201_create_ci_build_trace_sections.rb b/db/migrate/20170926102201_create_ci_build_trace_sections.rb deleted file mode 100644 index ad11b31b75d..00000000000 --- a/db/migrate/20170926102201_create_ci_build_trace_sections.rb +++ /dev/null @@ -1,19 +0,0 @@ -class CreateCiBuildTraceSections < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - def change - create_table :ci_build_trace_sections do |t| - t.datetime_with_timezone :date_start, null: false - t.datetime_with_timezone :date_end, null: false - t.integer :byte_start, limit: 8, null: false - t.integer :byte_end, limit: 8, null: false - t.references :project, null: false, index: true, foreign_key: { on_delete: :cascade } - t.integer :build_id, null: false - t.integer :section_name_id, null: false - end - - add_index :ci_build_trace_sections, [:build_id, :section_name_id], unique: true - end -end diff --git a/db/migrate/20170926134436_add_build_foreign_key_to_ci_build_trace_sections.rb b/db/migrate/20170926134436_add_build_foreign_key_to_ci_build_trace_sections.rb deleted file mode 100644 index d279463eb4b..00000000000 --- a/db/migrate/20170926134436_add_build_foreign_key_to_ci_build_trace_sections.rb +++ /dev/null @@ -1,15 +0,0 @@ -class AddBuildForeignKeyToCiBuildTraceSections < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - disable_ddl_transaction! - - def up - add_concurrent_foreign_key(:ci_build_trace_sections, :ci_builds, column: :build_id) - end - - def down - remove_foreign_key(:ci_build_trace_sections, column: :build_id) - end -end diff --git a/db/migrate/20170929123651_create_ci_build_trace_section_names.rb b/db/migrate/20170929123651_create_ci_build_trace_section_names.rb deleted file mode 100644 index 88f3e60699a..00000000000 --- a/db/migrate/20170929123651_create_ci_build_trace_section_names.rb +++ /dev/null @@ -1,19 +0,0 @@ -class CreateCiBuildTraceSectionNames < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - def up - create_table :ci_build_trace_section_names do |t| - t.references :project, null: false, foreign_key: { on_delete: :cascade } - t.string :name, null: false - end - - add_index :ci_build_trace_section_names, [:project_id, :name], unique: true - end - - def down - remove_foreign_key :ci_build_trace_section_names, column: :project_id - drop_table :ci_build_trace_section_names - end -end diff --git a/db/migrate/20170929123915_add_name_foreign_key_to_ci_build_trace_sections.rb b/db/migrate/20170929123915_add_name_foreign_key_to_ci_build_trace_sections.rb deleted file mode 100644 index 08422885a98..00000000000 --- a/db/migrate/20170929123915_add_name_foreign_key_to_ci_build_trace_sections.rb +++ /dev/null @@ -1,15 +0,0 @@ -class AddNameForeignKeyToCiBuildTraceSections < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - disable_ddl_transaction! - - def up - add_concurrent_foreign_key(:ci_build_trace_sections, :ci_build_trace_section_names, column: :section_name_id) - end - - def down - remove_foreign_key(:ci_build_trace_sections, column: :section_name_id) - end -end diff --git a/db/migrate/20171006090001_create_ci_build_trace_sections.rb b/db/migrate/20171006090001_create_ci_build_trace_sections.rb new file mode 100644 index 00000000000..ab5ef319618 --- /dev/null +++ b/db/migrate/20171006090001_create_ci_build_trace_sections.rb @@ -0,0 +1,19 @@ +class CreateCiBuildTraceSections < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + create_table :ci_build_trace_sections do |t| + t.references :project, null: false, index: true, foreign_key: { on_delete: :cascade } + t.datetime_with_timezone :date_start, null: false + t.datetime_with_timezone :date_end, null: false + t.integer :byte_start, limit: 8, null: false + t.integer :byte_end, limit: 8, null: false + t.integer :build_id, null: false + t.integer :section_name_id, null: false + end + + add_index :ci_build_trace_sections, [:build_id, :section_name_id], unique: true + end +end diff --git a/db/migrate/20171006090010_add_build_foreign_key_to_ci_build_trace_sections.rb b/db/migrate/20171006090010_add_build_foreign_key_to_ci_build_trace_sections.rb new file mode 100644 index 00000000000..d279463eb4b --- /dev/null +++ b/db/migrate/20171006090010_add_build_foreign_key_to_ci_build_trace_sections.rb @@ -0,0 +1,15 @@ +class AddBuildForeignKeyToCiBuildTraceSections < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_foreign_key(:ci_build_trace_sections, :ci_builds, column: :build_id) + end + + def down + remove_foreign_key(:ci_build_trace_sections, column: :build_id) + end +end diff --git a/db/migrate/20171006090100_create_ci_build_trace_section_names.rb b/db/migrate/20171006090100_create_ci_build_trace_section_names.rb new file mode 100644 index 00000000000..88f3e60699a --- /dev/null +++ b/db/migrate/20171006090100_create_ci_build_trace_section_names.rb @@ -0,0 +1,19 @@ +class CreateCiBuildTraceSectionNames < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + create_table :ci_build_trace_section_names do |t| + t.references :project, null: false, foreign_key: { on_delete: :cascade } + t.string :name, null: false + end + + add_index :ci_build_trace_section_names, [:project_id, :name], unique: true + end + + def down + remove_foreign_key :ci_build_trace_section_names, column: :project_id + drop_table :ci_build_trace_section_names + end +end diff --git a/db/migrate/20171006091000_add_name_foreign_key_to_ci_build_trace_sections.rb b/db/migrate/20171006091000_add_name_foreign_key_to_ci_build_trace_sections.rb new file mode 100644 index 00000000000..08422885a98 --- /dev/null +++ b/db/migrate/20171006091000_add_name_foreign_key_to_ci_build_trace_sections.rb @@ -0,0 +1,15 @@ +class AddNameForeignKeyToCiBuildTraceSections < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_foreign_key(:ci_build_trace_sections, :ci_build_trace_section_names, column: :section_name_id) + end + + def down + remove_foreign_key(:ci_build_trace_sections, column: :section_name_id) + end +end diff --git a/db/schema.rb b/db/schema.rb index 4f2424ff2ab..5f16e14139c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20171004121444) do +ActiveRecord::Schema.define(version: 20171006091000) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -215,11 +215,11 @@ ActiveRecord::Schema.define(version: 20171004121444) do add_index "ci_build_trace_section_names", ["project_id", "name"], name: "index_ci_build_trace_section_names_on_project_id_and_name", unique: true, using: :btree create_table "ci_build_trace_sections", force: :cascade do |t| + t.integer "project_id", null: false t.datetime_with_timezone "date_start", null: false t.datetime_with_timezone "date_end", null: false t.integer "byte_start", limit: 8, null: false t.integer "byte_end", limit: 8, null: false - t.integer "project_id", null: false t.integer "build_id", null: false t.integer "section_name_id", null: false end -- cgit v1.2.1 From c0cfc9ebd26583c444f2cce1a23f939bfa7d8969 Mon Sep 17 00:00:00 2001 From: Alessio Caiazza Date: Fri, 6 Oct 2017 12:33:10 +0200 Subject: Extract `Ci::Build#parse_trace_sections!` into a service --- app/models/ci/build.rb | 19 +------- .../extract_sections_from_build_trace_service.rb | 30 ++++++++++++ spec/models/ci/build_spec.rb | 23 +++------ ...tract_sections_from_build_trace_service_spec.rb | 55 ++++++++++++++++++++++ 4 files changed, 92 insertions(+), 35 deletions(-) create mode 100644 app/services/ci/extract_sections_from_build_trace_service.rb create mode 100644 spec/services/ci/extract_sections_from_build_trace_service_spec.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 72e646210a8..6ca46ae89c1 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -267,24 +267,7 @@ module Ci end def parse_trace_sections! - return false unless trace_sections.empty? - - sections = trace.extract_sections.map do |attr| - name = attr.delete(:name) - name_record = begin - project.build_trace_section_names.find_or_create_by!(name: name) - rescue ActiveRecord::RecordInvalid - project.build_trace_section_names.find_by!(name: name) - end - - attr.merge( - build_id: self.id, - project_id: self.project_id, - section_name_id: name_record.id) - end - - Gitlab::Database.bulk_insert(Ci::BuildTraceSection.table_name, sections) - true + ExtractSectionsFromBuildTraceService.new(project, user).execute(self) end def trace diff --git a/app/services/ci/extract_sections_from_build_trace_service.rb b/app/services/ci/extract_sections_from_build_trace_service.rb new file mode 100644 index 00000000000..75f9e0f897d --- /dev/null +++ b/app/services/ci/extract_sections_from_build_trace_service.rb @@ -0,0 +1,30 @@ +module Ci + class ExtractSectionsFromBuildTraceService < BaseService + def execute(build) + return false unless build.trace_sections.empty? + + Gitlab::Database.bulk_insert(BuildTraceSection.table_name, extract_sections(build)) + true + end + + private + + def find_or_create_name(name) + project.build_trace_section_names.find_or_create_by!(name: name) + rescue ActiveRecord::RecordInvalid + project.build_trace_section_names.find_by!(name: name) + end + + def extract_sections(build) + build.trace.extract_sections.map do |attr| + name = attr.delete(:name) + name_record = find_or_create_name(name) + + attr.merge( + build_id: build.id, + project_id: project.id, + section_name_id: name_record.id) + end + end + end +end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 7fe12f92a2a..06f76b5501e 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -322,24 +322,13 @@ describe Ci::Build do end describe '#parse_trace_sections!' do - context "when the build trace has sections markers," do - before do - build.trace.set(File.read(expand_fixture_path('trace/trace_with_sections'))) - end - - it "saves the correct extracted sections" do - expect(build.trace_sections).to be_empty - expect(build.parse_trace_sections!).to be(true) - expect(build.trace_sections).not_to be_empty - end - - it "fails if trace_sections isn't empty" do - expect(build.parse_trace_sections!).to be(true) - expect(build.trace_sections).not_to be_empty + it 'calls ExtractSectionsFromBuildTraceService' do + expect(Ci::ExtractSectionsFromBuildTraceService) + .to receive(:new).with(project, build.user).once.and_call_original + expect_any_instance_of(Ci::ExtractSectionsFromBuildTraceService) + .to receive(:execute).with(build).once - expect(build.parse_trace_sections!).to be(false) - expect(build.trace_sections).not_to be_empty - end + build.parse_trace_sections! end end diff --git a/spec/services/ci/extract_sections_from_build_trace_service_spec.rb b/spec/services/ci/extract_sections_from_build_trace_service_spec.rb new file mode 100644 index 00000000000..28f2fa7903a --- /dev/null +++ b/spec/services/ci/extract_sections_from_build_trace_service_spec.rb @@ -0,0 +1,55 @@ +require 'spec_helper' + +describe Ci::ExtractSectionsFromBuildTraceService, '#execute' do + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:build) { create(:ci_build, project: project) } + + subject { described_class.new(project, user) } + + shared_examples 'build trace has sections markers' do + before do + build.trace.set(File.read(expand_fixture_path('trace/trace_with_sections'))) + end + + it 'saves the correct extracted sections' do + expect(build.trace_sections).to be_empty + expect(subject.execute(build)).to be(true) + expect(build.trace_sections).not_to be_empty + end + + it "fails if trace_sections isn't empty" do + expect(subject.execute(build)).to be(true) + expect(build.trace_sections).not_to be_empty + + expect(subject.execute(build)).to be(false) + expect(build.trace_sections).not_to be_empty + end + end + + shared_examples 'build trace has no sections markers' do + before do + build.trace.set('no markerts') + end + + it 'extracts no sections' do + expect(build.trace_sections).to be_empty + expect(subject.execute(build)).to be(true) + expect(build.trace_sections).to be_empty + end + end + + context 'when the build has no user' do + it_behaves_like 'build trace has sections markers' + it_behaves_like 'build trace has no sections markers' + end + + context 'when the build has a valid user' do + before do + build.user = user + end + + it_behaves_like 'build trace has sections markers' + it_behaves_like 'build trace has no sections markers' + end +end -- cgit v1.2.1 From 60e9ba21e9cec68629d39a554747607f3049fd88 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Fri, 6 Oct 2017 18:12:42 +0100 Subject: Removes unnecessary load of LinkedTabs, Pipelines and MiniPipelineGraph classes --- app/assets/javascripts/main.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/app/assets/javascripts/main.js b/app/assets/javascripts/main.js index 24abc5c5c9e..2c1dcc374ff 100644 --- a/app/assets/javascripts/main.js +++ b/app/assets/javascripts/main.js @@ -40,7 +40,6 @@ import './commit/file'; import './commit/image_file'; // lib/utils -import './lib/utils/bootstrap_linked_tabs'; import { handleLocationHash } from './lib/utils/common_utils'; import './lib/utils/datetime_utility'; import './lib/utils/pretty_time'; @@ -111,7 +110,6 @@ import './merge_request'; import './merge_request_tabs'; import './milestone'; import './milestone_select'; -import './mini_pipeline_graph_dropdown'; import './namespace_select'; import './new_branch_form'; import './new_commit_form'; @@ -119,7 +117,6 @@ import './notes'; import './notifications_dropdown'; import './notifications_form'; import './pager'; -import './pipelines'; import './preview_markdown'; import './project'; import './project_avatar'; -- cgit v1.2.1 From 2ae20000a3c1a51a05c6048393ce2031a15ab6a8 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Thu, 28 Sep 2017 15:54:16 -0700 Subject: Remove nav images; remove old nav styles; create new file for other navigation elements --- app/assets/images/new_nav.png | Bin 14322 -> 0 bytes app/assets/images/old_nav.png | Bin 25617 -> 0 bytes app/assets/stylesheets/framework.scss | 1 + app/assets/stylesheets/framework/blocks.scss | 4 + app/assets/stylesheets/framework/layout.scss | 4 - app/assets/stylesheets/framework/nav.scss | 515 --------------------- app/assets/stylesheets/framework/new-nav.scss | 5 - .../framework/secondary-navigation-elements.scss | 337 ++++++++++++++ app/assets/stylesheets/pages/boards.scss | 1 - app/views/users/show.html.haml | 2 +- 10 files changed, 343 insertions(+), 526 deletions(-) delete mode 100644 app/assets/images/new_nav.png delete mode 100644 app/assets/images/old_nav.png create mode 100644 app/assets/stylesheets/framework/secondary-navigation-elements.scss diff --git a/app/assets/images/new_nav.png b/app/assets/images/new_nav.png deleted file mode 100644 index f98ca15d787..00000000000 Binary files a/app/assets/images/new_nav.png and /dev/null differ diff --git a/app/assets/images/old_nav.png b/app/assets/images/old_nav.png deleted file mode 100644 index 23fae7aa19e..00000000000 Binary files a/app/assets/images/old_nav.png and /dev/null differ diff --git a/app/assets/stylesheets/framework.scss b/app/assets/stylesheets/framework.scss index e8037c77aab..09a29d41823 100644 --- a/app/assets/stylesheets/framework.scss +++ b/app/assets/stylesheets/framework.scss @@ -34,6 +34,7 @@ @import "framework/new-nav"; @import "framework/pagination"; @import "framework/panels"; +@import "framework/secondary-navigation-elements"; @import "framework/selects"; @import "framework/sidebar"; @import "framework/new-sidebar"; diff --git a/app/assets/stylesheets/framework/blocks.scss b/app/assets/stylesheets/framework/blocks.scss index 1d72a70f0f5..d63145771c9 100644 --- a/app/assets/stylesheets/framework/blocks.scss +++ b/app/assets/stylesheets/framework/blocks.scss @@ -207,6 +207,10 @@ &.user-cover-block { padding: 24px 0 0; + + .nav-links { + justify-content: center; + } } .group-info { diff --git a/app/assets/stylesheets/framework/layout.scss b/app/assets/stylesheets/framework/layout.scss index bd521028c44..69d19ea2962 100644 --- a/app/assets/stylesheets/framework/layout.scss +++ b/app/assets/stylesheets/framework/layout.scss @@ -25,10 +25,6 @@ body { .content-wrapper { padding-bottom: 100px; - - &:not(.page-with-layout-nav) { - margin-top: $header-height; - } } .container { diff --git a/app/assets/stylesheets/framework/nav.scss b/app/assets/stylesheets/framework/nav.scss index f8777d1fd9d..ca206ce818f 100644 --- a/app/assets/stylesheets/framework/nav.scss +++ b/app/assets/stylesheets/framework/nav.scss @@ -1,255 +1,3 @@ - - -.nav-links { - display: flex; - padding: 0; - margin: 0; - list-style: none; - height: auto; - border-bottom: 1px solid $border-color; - - li { - display: flex; - - a { - padding: $gl-btn-padding; - padding-bottom: 11px; - font-size: 14px; - line-height: 28px; - color: $gl-text-color-secondary; - border-bottom: 2px solid transparent; - white-space: nowrap; - - &:hover, - &:active, - &:focus { - text-decoration: none; - border-bottom: 2px solid $gray-darkest; - color: $black; - - .badge { - color: $black; - } - } - } - - &.active a { - border-bottom: 2px solid $link-underline-blue; - color: $black; - font-weight: $gl-font-weight-bold; - - .badge { - color: $black; - } - } - } - - &.sub-nav { - text-align: center; - background-color: $gray-normal; - - .container-fluid { - background-color: $gray-normal; - margin-bottom: 0; - display: flex; - } - - li { - &.active a { - border-bottom: none; - color: $link-underline-blue; - } - - a { - margin: 0; - padding: 11px 10px 9px; - - &:hover, - &:active, - &:focus { - border-color: transparent; - } - } - } - } -} - -.top-area { - @include clearfix; - border-bottom: 1px solid $border-color; - - .nav-text { - padding-top: 16px; - padding-bottom: 11px; - display: inline-block; - line-height: 28px; - white-space: normal; - - /* Small devices (phones, tablets, 768px and lower) */ - @media (max-width: $screen-xs-max) { - width: 100%; - } - } - - .nav-search { - display: inline-block; - width: 100%; - padding: 11px 0; - - /* Small devices (phones, tablets, 768px and lower) */ - @media (min-width: $screen-sm-min) { - width: 50%; - } - } - - .nav-links { - margin-bottom: 0; - border-bottom: none; - float: left; - - &.wide { - width: 100%; - display: block; - } - - &.scrolling-tabs { - float: left; - } - - li a { - padding: 16px 15px 11px; - } - - /* Small devices (phones, tablets, 768px and lower) */ - @media (max-width: $screen-xs-max) { - width: 100%; - } - } - - .nav-controls { - @include new-style-dropdown; - - display: inline-block; - float: right; - text-align: right; - padding: 11px 0; - margin-bottom: 0; - - > .btn, - > .btn-container, - > .dropdown, - > input, - > form { - margin-right: $gl-padding-top; - display: inline-block; - vertical-align: top; - - &:last-child { - margin-right: 0; - float: right; - } - } - - &.nav-controls-new-nav { - > .dropdown { - margin-right: 0; - } - } - - > .btn-grouped { - float: none; - } - - .icon-label { - display: none; - } - - input { - display: inline-block; - position: relative; - - /* Medium devices (desktops, 992px and up) */ - @media (min-width: $screen-md-min) { width: 200px; } - - /* Large devices (large desktops, 1200px and up) */ - @media (min-width: $screen-lg-min) { width: 250px; } - - &.input-short { - /* Medium devices (desktops, 992px and up) */ - @media (min-width: $screen-md-min) { width: 170px; } - - /* Large devices (large desktops, 1200px and up) */ - @media (min-width: $screen-lg-min) { width: 210px; } - } - } - - @media (max-width: $screen-xs-max) { - padding-bottom: 0; - width: 100%; - - .btn, - form, - .dropdown, - .dropdown-toggle, - .dropdown-menu-toggle, - .form-control { - margin: 0 0 10px; - display: block; - width: 100%; - } - - form { - display: block; - height: auto; - margin-bottom: 14px; - - input { - width: 100%; - margin: 0 0 10px; - } - } - - .input-short { - width: 100%; - } - - .icon-label { - display: inline-block; - } - - // Applies on /dashboard/issues - .project-item-select-holder { - margin: 0; - } - } - } - - &.adjust { - .nav-text, - .nav-controls { - width: auto; - - @media (max-width: $screen-xs-max) { - width: 100%; - } - } - } - - &.multi-line { - .nav-text { - line-height: 20px; - } - - .nav-controls { - padding: 17px 0; - } - } - - pre { - width: 100%; - } -} - .project-item-select-holder.btn-group { display: flex; max-width: 350px; @@ -280,272 +28,9 @@ @media(max-width: $screen-xs-max) { max-width: 250px; } - } } .new-project-item-select-button .fa-caret-down { margin-left: 2px; } - -.layout-nav { - width: 100%; - background: $gray-light; - border-bottom: 1px solid $border-color; - transition: padding $sidebar-transition-duration; - text-align: center; - margin-top: $new-navbar-height; - - .container-fluid { - position: relative; - - .nav-control { - @media (max-width: $screen-sm-max) { - margin-right: 2px; - } - } - } - - .controls { - float: right; - padding: 7px 0 0; - - i { - color: $layout-link-gray; - } - - .fa-rss, - .fa-cog { - font-size: 16px; - } - - .fa-caret-down { - margin-left: 5px; - color: $gl-text-color-secondary; - } - - .dropdown { - position: absolute; - top: 7px; - right: 15px; - z-index: 300; - - li.active { - font-weight: $gl-font-weight-bold; - } - } - } - - .nav-links { - border-bottom: none; - height: 51px; - - @media (min-width: $screen-sm-min) { - justify-content: center; - } - - li { - a { - padding-top: 10px; - } - } - } -} - -.with-performance-bar .layout-nav { - margin-top: $header-height + $performance-bar-height; -} - -.scrolling-tabs-container { - position: relative; - - .merge-request-tabs-container & { - overflow: hidden; - } - - .nav-links { - @include scrolling-links(); - } - - .fade-right { - @include fade(left, $gray-light); - right: -5px; - - .fa { - right: -7px; - } - } - - .fade-left { - @include fade(right, $gray-light); - left: -5px; - text-align: center; - - .fa { - left: -7px; - } - } - - &.sub-nav-scroll { - - .fade-right { - @include fade(left, $gray-normal); - right: 0; - - .fa { - right: -23px; - } - } - - .fade-left { - @include fade(right, $gray-normal); - left: 0; - - .fa { - left: 10px; - } - } - } -} - -.nav-block { - position: relative; - - .nav-links { - @include scrolling-links(); - - .fade-right { - @include fade(left, $white-light); - right: -5px; - - .fa { - right: -7px; - } - } - - .fade-left { - @include fade(right, $white-light); - left: -5px; - - .fa { - left: -7px; - } - } - } -} - -.page-with-layout-nav { - .right-sidebar { - top: ($header-height + 1) * 2; - } - - &.page-with-sub-nav { - .right-sidebar { - top: ($header-height + 1) * 3; - - &.affix { - top: $header-height; - } - } - } -} - -.with-performance-bar .page-with-layout-nav { - .right-sidebar { - top: ($header-height + 1) * 2 + $performance-bar-height; - } - - &.page-with-sub-nav { - .right-sidebar { - top: ($header-height + 1) * 3 + $performance-bar-height; - - &.affix { - top: $header-height + $performance-bar-height; - } - } - } -} - -.nav-block { - &.activities { - border-bottom: 1px solid $border-color; - - .nav-links { - border-bottom: none; - } - } -} - -@media (max-width: $screen-xs-max) { - .top-area { - flex-flow: row wrap; - - .nav-controls { - $controls-margin: $btn-xs-side-margin - 2px; - flex: 0 0 100%; - - &.controls-flex { - display: flex; - flex-flow: row wrap; - align-items: center; - justify-content: center; - padding: 0 0 $gl-padding-top; - } - - .controls-item, - .controls-item-full, - .controls-item:last-child { - flex: 1 1 35%; - display: block; - width: 100%; - margin: $controls-margin; - - .btn, - .dropdown { - margin: 0; - } - } - - .controls-item-full { - flex: 1 1 100%; - } - } - } -} - -.inner-page-scroll-tabs { - position: relative; - - .fade-right { - @include fade(left, $white-light); - right: 0; - text-align: right; - - .fa { - right: 5px; - } - } - - .fade-left { - @include fade(right, $white-light); - left: 0; - text-align: left; - - .fa { - left: 5px; - } - } - - .fade-right, - .fade-left { - top: 16px; - bottom: auto; - } - - &.is-smaller { - .fade-right, - .fade-left { - top: 11px; - } - } -} diff --git a/app/assets/stylesheets/framework/new-nav.scss b/app/assets/stylesheets/framework/new-nav.scss index 7899be2c2d3..3663ebf3039 100644 --- a/app/assets/stylesheets/framework/new-nav.scss +++ b/app/assets/stylesheets/framework/new-nav.scss @@ -1,8 +1,3 @@ -@import "framework/variables"; -@import 'framework/tw_bootstrap_variables'; -@import "bootstrap/variables"; -@import "framework/mixins"; - .content-wrapper.page-with-new-nav { margin-top: $new-navbar-height; } diff --git a/app/assets/stylesheets/framework/secondary-navigation-elements.scss b/app/assets/stylesheets/framework/secondary-navigation-elements.scss new file mode 100644 index 00000000000..1b9a26512b4 --- /dev/null +++ b/app/assets/stylesheets/framework/secondary-navigation-elements.scss @@ -0,0 +1,337 @@ +// For tabbed navigation links, scrolling tabs, etc. For all top/main navigation, +// please check nav.scss +.nav-links { + display: flex; + padding: 0; + margin: 0; + list-style: none; + height: auto; + + li { + display: flex; + + a { + font-size: 14px; + line-height: 28px; + color: $gl-text-color-secondary; + border-bottom: 2px solid transparent; + white-space: nowrap; + + &:hover, + &:active, + &:focus { + text-decoration: none; + color: $black; + + .badge { + color: $black; + } + } + } + + &.active a { + color: $black; + font-weight: $gl-font-weight-bold; + + .badge { + color: $black; + } + } + } +} + +.top-area { + @include clearfix; + border-bottom: 1px solid $border-color; + + .nav-text { + padding-top: 16px; + padding-bottom: 11px; + display: inline-block; + line-height: 28px; + white-space: normal; + + /* Small devices (phones, tablets, 768px and lower) */ + @media (max-width: $screen-xs-max) { + width: 100%; + } + } + + .nav-links { + margin-bottom: 0; + border-bottom: none; + float: left; + + &.wide { + width: 100%; + display: block; + } + + &.scrolling-tabs { + float: left; + } + + li a { + padding: 16px 15px 11px; + } + + /* Small devices (phones, tablets, 768px and lower) */ + @media (max-width: $screen-xs-max) { + width: 100%; + } + } + + .nav-controls { + @include new-style-dropdown; + + display: inline-block; + float: right; + text-align: right; + padding: 11px 0; + margin-bottom: 0; + + > .btn, + > .btn-container, + > .dropdown, + > input, + > form { + margin-right: $gl-padding-top; + display: inline-block; + vertical-align: top; + + &:last-child { + margin-right: 0; + float: right; + } + } + + > .btn-grouped { + float: none; + } + + .icon-label { + display: none; + } + + input { + display: inline-block; + position: relative; + + /* Medium devices (desktops, 992px and up) */ + @media (min-width: $screen-md-min) { width: 200px; } + + /* Large devices (large desktops, 1200px and up) */ + @media (min-width: $screen-lg-min) { width: 250px; } + + &.input-short { + /* Medium devices (desktops, 992px and up) */ + @media (min-width: $screen-md-min) { width: 170px; } + + /* Large devices (large desktops, 1200px and up) */ + @media (min-width: $screen-lg-min) { width: 210px; } + } + } + + @media (max-width: $screen-xs-max) { + padding-bottom: 0; + width: 100%; + + .btn, + form, + .dropdown, + .dropdown-toggle, + .dropdown-menu-toggle, + .form-control { + margin: 0 0 10px; + display: block; + width: 100%; + } + + form { + display: block; + height: auto; + margin-bottom: 14px; + + input { + width: 100%; + margin: 0 0 10px; + } + } + + .input-short { + width: 100%; + } + + .icon-label { + display: inline-block; + } + + // Applies on /dashboard/issues + .project-item-select-holder { + margin: 0; + } + } + } + + &.adjust { + .nav-text, + .nav-controls { + width: auto; + + @media (max-width: $screen-xs-max) { + width: 100%; + } + } + } + + &.multi-line { + .nav-text { + line-height: 20px; + } + + .nav-controls { + padding: 17px 0; + } + } + + pre { + width: 100%; + } + + @media (max-width: $screen-xs-max) { + flex-flow: row wrap; + + .nav-controls { + $controls-margin: $btn-xs-side-margin - 2px; + flex: 0 0 100%; + + &.controls-flex { + display: flex; + flex-flow: row wrap; + align-items: center; + justify-content: center; + padding: 0 0 $gl-padding-top; + } + + .controls-item, + .controls-item-full, + .controls-item:last-child { + flex: 1 1 35%; + display: block; + width: 100%; + margin: $controls-margin; + + .btn, + .dropdown { + margin: 0; + } + } + + .controls-item-full { + flex: 1 1 100%; + } + } + } +} + +.scrolling-tabs-container { + position: relative; + + .merge-request-tabs-container & { + overflow: hidden; + } + + .nav-links { + @include scrolling-links(); + } + + .fade-right { + @include fade(left, $gray-light); + right: -5px; + + .fa { + right: -7px; + } + } + + .fade-left { + @include fade(right, $gray-light); + left: -5px; + text-align: center; + + .fa { + left: -7px; + } + } +} + +.inner-page-scroll-tabs { + position: relative; + + .fade-right { + @include fade(left, $white-light); + right: 0; + text-align: right; + + .fa { + right: 5px; + } + } + + .fade-left { + @include fade(right, $white-light); + left: 0; + text-align: left; + + .fa { + left: 5px; + } + } + + .fade-right, + .fade-left { + top: 16px; + bottom: auto; + } + + &.is-smaller { + .fade-right, + .fade-left { + top: 11px; + } + } +} + +.nav-block { + position: relative; + + .nav-links { + @include scrolling-links(); + + .fade-right { + @include fade(left, $white-light); + right: -5px; + + .fa { + right: -7px; + } + } + + .fade-left { + @include fade(right, $white-light); + left: -5px; + + .fa { + left: -7px; + } + } + } + + &.activities { + border-bottom: 1px solid $border-color; + + .nav-links { + border-bottom: none; + } + } +} diff --git a/app/assets/stylesheets/pages/boards.scss b/app/assets/stylesheets/pages/boards.scss index 3305a482a0d..ca61f7a30c3 100644 --- a/app/assets/stylesheets/pages/boards.scss +++ b/app/assets/stylesheets/pages/boards.scss @@ -414,7 +414,6 @@ margin: 5px; } -.page-with-layout-nav.page-with-sub-nav .issue-boards-sidebar, .page-with-new-sidebar.page-with-sidebar .issue-boards-sidebar { .issuable-sidebar-header { position: relative; diff --git a/app/views/users/show.html.haml b/app/views/users/show.html.haml index d0ffcc88d43..6c3cd6ecefe 100644 --- a/app/views/users/show.html.haml +++ b/app/views/users/show.html.haml @@ -9,7 +9,7 @@ = auto_discovery_link_tag(:atom, user_url(@user, format: :atom), title: "#{@user.name} activity") .user-profile - .cover-block.user-cover-block.layout-nav + .cover-block.user-cover-block.top-area .cover-controls - if @user == current_user = link_to profile_path, class: 'btn btn-gray has-tooltip', title: 'Edit profile', 'aria-label': 'Edit profile' do -- cgit v1.2.1 From af86b5dec1ac8ea7120cc5eb51320bf21eea761a Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Thu, 28 Sep 2017 16:17:04 -0700 Subject: Fix scrolling tabs --- app/assets/stylesheets/framework/blocks.scss | 6 ++++++ app/assets/stylesheets/framework/secondary-navigation-elements.scss | 2 ++ 2 files changed, 8 insertions(+) diff --git a/app/assets/stylesheets/framework/blocks.scss b/app/assets/stylesheets/framework/blocks.scss index d63145771c9..dbd990f84c1 100644 --- a/app/assets/stylesheets/framework/blocks.scss +++ b/app/assets/stylesheets/framework/blocks.scss @@ -210,6 +210,12 @@ .nav-links { justify-content: center; + width: 100%; + float: none; + + &.scrolling-tabs { + float: none; + } } } diff --git a/app/assets/stylesheets/framework/secondary-navigation-elements.scss b/app/assets/stylesheets/framework/secondary-navigation-elements.scss index 1b9a26512b4..5be6bae3368 100644 --- a/app/assets/stylesheets/framework/secondary-navigation-elements.scss +++ b/app/assets/stylesheets/framework/secondary-navigation-elements.scss @@ -11,6 +11,8 @@ display: flex; a { + padding: $gl-btn-padding; + padding-bottom: 11px; font-size: 14px; line-height: 28px; color: $gl-text-color-secondary; -- cgit v1.2.1 From 314cfc39a7c3832f212d9837970f6cc59c436a50 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Thu, 28 Sep 2017 16:51:29 -0700 Subject: Fixes --- app/assets/stylesheets/framework/header.scss | 94 ---------------------- app/assets/stylesheets/framework/nav.scss | 35 -------- app/assets/stylesheets/framework/new-nav.scss | 92 +++++++++++++++++++++ .../framework/secondary-navigation-elements.scss | 37 +++++++++ app/assets/stylesheets/pages/search.scss | 1 + 5 files changed, 130 insertions(+), 129 deletions(-) diff --git a/app/assets/stylesheets/framework/header.scss b/app/assets/stylesheets/framework/header.scss index d932ea8794f..63eed636476 100644 --- a/app/assets/stylesheets/framework/header.scss +++ b/app/assets/stylesheets/framework/header.scss @@ -6,8 +6,6 @@ header { @include new-style-dropdown; - transition: padding $sidebar-transition-duration; - &.navbar-empty { height: $header-height; background: $white-light; @@ -313,95 +311,3 @@ header { color: $red-500; } } - -.with-performance-bar header.navbar-gitlab { - top: $performance-bar-height; -} - -.navbar-nav { - li { - .badge { - position: inherit; - font-weight: $gl-font-weight-normal; - margin-left: -6px; - font-size: 11px; - color: $white-light; - padding: 0 5px; - line-height: 12px; - border-radius: 7px; - box-shadow: 0 1px 0 rgba($gl-header-color, .2); - - &.issues-count { - background-color: $green-500; - } - - &.merge-requests-count { - background-color: $orange-600; - } - - &.todos-count { - background-color: $blue-500; - } - } - } -} - -@media (max-width: $screen-xs-max) { - header .container-fluid { - font-size: 18px; - - .navbar-nav { - display: table; - table-layout: fixed; - width: 100%; - margin: 0; - text-align: right; - } - - .navbar-collapse { - padding-left: 5px; - - .nav > li:not(.hidden-xs) { - display: table-cell !important; - width: 25%; - - a { - margin-right: 8px; - } - } - } - } - - .header-user-dropdown-toggle { - text-align: center; - } - - .header-user-avatar { - float: none; - } -} - -.header-user { - .dropdown-menu-nav { - width: auto; - min-width: 140px; - margin-top: -5px; - color: $gl-text-color; - left: auto; - - .current-user { - padding: 5px 18px; - - .user-name { - display: block; - } - } - } -} - -.header-user-avatar { - float: left; - margin-right: 5px; - border-radius: 50%; - border: 1px solid $avatar-border; -} diff --git a/app/assets/stylesheets/framework/nav.scss b/app/assets/stylesheets/framework/nav.scss index ca206ce818f..8b137891791 100644 --- a/app/assets/stylesheets/framework/nav.scss +++ b/app/assets/stylesheets/framework/nav.scss @@ -1,36 +1 @@ -.project-item-select-holder.btn-group { - display: flex; - max-width: 350px; - overflow: hidden; - float: right; - .new-project-item-link { - white-space: nowrap; - overflow: hidden; - text-overflow: ellipsis; - } - - .new-project-item-select-button { - width: 32px; - } -} - -.empty-state .project-item-select-holder.btn-group { - float: none; - display: inline-block; - - .btn { - // overrides styles applied to plain `.empty-state .btn` - margin: 10px 0; - max-width: 300px; - width: auto; - - @media(max-width: $screen-xs-max) { - max-width: 250px; - } - } -} - -.new-project-item-select-button .fa-caret-down { - margin-left: 2px; -} diff --git a/app/assets/stylesheets/framework/new-nav.scss b/app/assets/stylesheets/framework/new-nav.scss index 3663ebf3039..0ee07b18b1f 100644 --- a/app/assets/stylesheets/framework/new-nav.scss +++ b/app/assets/stylesheets/framework/new-nav.scss @@ -397,3 +397,95 @@ header.navbar-gitlab-new { background-color: $white-light; } } + +.navbar-nav { + li { + .badge { + position: inherit; + font-weight: $gl-font-weight-normal; + margin-left: -6px; + font-size: 11px; + color: $white-light; + padding: 0 5px; + line-height: 12px; + border-radius: 7px; + box-shadow: 0 1px 0 rgba($gl-header-color, .2); + + &.issues-count { + background-color: $green-500; + } + + &.merge-requests-count { + background-color: $orange-600; + } + + &.todos-count { + background-color: $blue-500; + } + } + } +} + +@media (max-width: $screen-xs-max) { + header .container-fluid { + font-size: 18px; + + .navbar-nav { + display: table; + table-layout: fixed; + width: 100%; + margin: 0; + text-align: right; + } + + .navbar-collapse { + padding-left: 5px; + + .nav > li:not(.hidden-xs) { + display: table-cell !important; + width: 25%; + + a { + margin-right: 8px; + } + } + } + } + + .header-user-dropdown-toggle { + text-align: center; + } + + .header-user-avatar { + float: none; + } +} + +.header-user { + .dropdown-menu-nav { + width: auto; + min-width: 140px; + margin-top: -5px; + color: $gl-text-color; + left: auto; + + .current-user { + padding: 5px 18px; + + .user-name { + display: block; + } + } + } +} + +.header-user-avatar { + float: left; + margin-right: 5px; + border-radius: 50%; + border: 1px solid $avatar-border; +} + +.with-performance-bar header.navbar-gitlab { + top: $performance-bar-height; +} diff --git a/app/assets/stylesheets/framework/secondary-navigation-elements.scss b/app/assets/stylesheets/framework/secondary-navigation-elements.scss index 5be6bae3368..5c96b3b78e7 100644 --- a/app/assets/stylesheets/framework/secondary-navigation-elements.scss +++ b/app/assets/stylesheets/framework/secondary-navigation-elements.scss @@ -337,3 +337,40 @@ } } } + +.project-item-select-holder.btn-group { + display: flex; + max-width: 350px; + overflow: hidden; + float: right; + + .new-project-item-link { + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; + } + + .new-project-item-select-button { + width: 32px; + } +} + +.empty-state .project-item-select-holder.btn-group { + float: none; + display: inline-block; + + .btn { + // overrides styles applied to plain `.empty-state .btn` + margin: 10px 0; + max-width: 300px; + width: auto; + + @media(max-width: $screen-xs-max) { + max-width: 250px; + } + } +} + +.new-project-item-select-button .fa-caret-down { + margin-left: 2px; +} diff --git a/app/assets/stylesheets/pages/search.scss b/app/assets/stylesheets/pages/search.scss index 89ebe3f9917..db0a04a5eb3 100644 --- a/app/assets/stylesheets/pages/search.scss +++ b/app/assets/stylesheets/pages/search.scss @@ -47,6 +47,7 @@ input[type="checkbox"]:hover { } .location-badge { + height: 32px; font-size: 12px; margin: -4px 4px -4px -4px; line-height: 25px; -- cgit v1.2.1 From 940e702468ecd8c811dc2f2f4602c38ebffd9742 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Fri, 29 Sep 2017 14:03:50 -0700 Subject: Start moving new nav styles into header.scss --- app/assets/javascripts/shortcuts.js | 15 -- app/assets/stylesheets/framework/animations.scss | 3 +- app/assets/stylesheets/framework/dropdowns.scss | 2 +- app/assets/stylesheets/framework/gitlab-theme.scss | 4 +- app/assets/stylesheets/framework/header.scss | 195 ++++++--------------- app/assets/stylesheets/framework/new-nav.scss | 86 +-------- app/assets/stylesheets/framework/new-sidebar.scss | 10 +- app/assets/stylesheets/framework/sidebar.scss | 6 +- app/assets/stylesheets/framework/variables.scss | 3 +- app/assets/stylesheets/pages/builds.scss | 8 +- app/assets/stylesheets/pages/issuable.scss | 6 +- app/assets/stylesheets/pages/merge_requests.scss | 4 +- app/views/layouts/header/_default.html.haml | 2 +- 13 files changed, 76 insertions(+), 268 deletions(-) diff --git a/app/assets/javascripts/shortcuts.js b/app/assets/javascripts/shortcuts.js index e754f6c4460..f63b99ba1c5 100644 --- a/app/assets/javascripts/shortcuts.js +++ b/app/assets/javascripts/shortcuts.js @@ -18,23 +18,8 @@ import findAndFollowLink from './shortcuts_dashboard_navigation'; Mousetrap.bind('f', (e => this.focusFilter(e))); Mousetrap.bind('p b', this.onTogglePerfBar); - const $globalDropdownMenu = $('.global-dropdown-menu'); - const $globalDropdownToggle = $('.global-dropdown-toggle'); const findFileURL = document.body.dataset.findFile; - $('.global-dropdown').on('hide.bs.dropdown', () => { - $globalDropdownMenu.removeClass('shortcuts'); - }); - - Mousetrap.bind('n', () => { - $globalDropdownMenu.toggleClass('shortcuts'); - $globalDropdownToggle.trigger('click'); - - if (!$globalDropdownMenu.is(':visible')) { - $globalDropdownToggle.blur(); - } - }); - Mousetrap.bind('shift+t', () => findAndFollowLink('.shortcuts-todos')); Mousetrap.bind('shift+a', () => findAndFollowLink('.dashboard-shortcuts-activity')); Mousetrap.bind('shift+i', () => findAndFollowLink('.dashboard-shortcuts-issues')); diff --git a/app/assets/stylesheets/framework/animations.scss b/app/assets/stylesheets/framework/animations.scss index 667b73e150d..f0e6b23757f 100644 --- a/app/assets/stylesheets/framework/animations.scss +++ b/app/assets/stylesheets/framework/animations.scss @@ -115,8 +115,7 @@ @return $unfoldedTransition; } -.btn, -.global-dropdown-toggle { +.btn { @include transition(background-color, border-color, color, box-shadow); } diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index fa92d4ccf4f..9dcf332eee2 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -889,7 +889,7 @@ @include new-style-dropdown('.breadcrumbs-list .dropdown '); @include new-style-dropdown('.js-namespace-select + '); -header.navbar-gitlab-new .header-content .dropdown-menu.projects-dropdown-menu { +header.header-content .dropdown-menu.projects-dropdown-menu { padding: 0; } diff --git a/app/assets/stylesheets/framework/gitlab-theme.scss b/app/assets/stylesheets/framework/gitlab-theme.scss index a6bdcf46aa7..45cf9b252a3 100644 --- a/app/assets/stylesheets/framework/gitlab-theme.scss +++ b/app/assets/stylesheets/framework/gitlab-theme.scss @@ -5,7 +5,7 @@ @mixin gitlab-theme($color-100, $color-200, $color-500, $color-700, $color-800, $color-900, $color-alternate) { // Header - header.navbar-gitlab-new { + header { background-color: $color-900; .navbar-collapse { @@ -200,7 +200,7 @@ body { &.ui_light { @include gitlab-theme($theme-gray-900, $theme-gray-700, $theme-gray-800, $theme-gray-700, $theme-gray-700, $theme-gray-100, $theme-gray-700); - header.navbar-gitlab-new { + header { background-color: $theme-gray-100; box-shadow: 0 2px 0 0 $border-color; diff --git a/app/assets/stylesheets/framework/header.scss b/app/assets/stylesheets/framework/header.scss index 63eed636476..c10e3467bef 100644 --- a/app/assets/stylesheets/framework/header.scss +++ b/app/assets/stylesheets/framework/header.scss @@ -27,84 +27,28 @@ header { z-index: 1000; margin-bottom: 0; min-height: $header-height; - background-color: $gray-light; border: none; border-bottom: 1px solid $border-color; position: fixed; top: 0; left: 0; right: 0; - color: $gl-text-color-secondary; border-radius: 0; - @media (max-width: $screen-xs-min) { - padding: 0 16px; - } - - &.with-horizontal-nav { - border-bottom: 0; + .logo-text { + line-height: initial; - .navbar-border { - height: 1px; - position: absolute; - right: 0; - left: 0; - bottom: -1px; - background-color: $border-color; - opacity: 0; + svg { + width: 55px; + height: 14px; + margin: 0; + fill: $white-light; } } .container-fluid { - width: 100% !important; - filter: none; padding: 0; - .nav > li > a { - color: currentColor; - font-size: 18px; - padding: 0; - margin: (($header-height - 28) / 2) 3px; - margin-left: 8px; - height: 28px; - min-width: 32px; - line-height: 28px; - text-align: center; - - &.header-user-dropdown-toggle { - margin-left: 14px; - - &:hover, - &:focus, - &:active { - .header-user-avatar { - border-color: rgba($avatar-border, .2); - } - } - } - - &:hover, - &:focus, - &:active { - background-color: transparent; - color: $gl-text-color; - - svg { - fill: $gl-text-color; - } - } - - .fa-caret-down { - font-size: 14px; - } - - .fa-chevron-down { - position: relative; - top: -3px; - font-size: 10px; - } - } - .user-counter { svg { margin-right: 3px; @@ -112,36 +56,34 @@ header { } .navbar-toggle { - color: $nav-toggle-gray; - margin: 5px 0; - border-radius: 0; - right: -10px; - padding: 6px 10px; - - &:hover { - background-color: $white-normal; - } + min-width: 45px; + padding: 4px $gl-padding; + margin-right: -7px; + font-size: 14px; + text-align: center; + color: currentColor; + &:hover, + &:focus, &.active { - color: $gl-text-color-secondary; + color: currentColor; + background-color: transparent; } } } } - &.navbar-gitlab-new { - .close-icon { + .fa-times { + display: none; + } + + .menu-expanded { + .fa-ellipsis-v { display: none; } - .menu-expanded { - .more-icon { - display: none; - } - - .close-icon { - display: block; - } + .fa-times { + display: block; } } @@ -160,29 +102,11 @@ header { } } - .global-dropdown-toggle { - margin: 7px 0; - font-size: 18px; - padding: 6px 10px; - border: none; - background-color: $gray-light; - - &:hover { - background-color: $white-normal; - } - - &:focus { - outline: none; - background-color: $white-normal; - } - } - .header-content { display: flex; - justify-content: space-between; position: relative; min-height: $header-height; - padding-left: 30px; + padding-left: 0; &.menu-expanded { @media (max-width: $screen-xs-max) { @@ -218,38 +142,32 @@ header { } } - .group-name-toggle { - margin: 3px 5px; - } - - .group-title { - &.is-hidden { - .hidable:not(:last-of-type) { - display: none; - } - } - } - .title-container { display: flex; - align-items: flex-start; + -webkit-align-items: stretch; + align-items: stretch; + -webkit-flex: 1 1 auto; flex: 1 1 auto; - padding-top: 14px; - overflow: hidden; + padding-top: 0; + overflow: visible; } .title { + padding-right: 0; + color: currentColor; + display: -webkit-flex; + display: flex; position: relative; - padding-right: 20px; margin: 0; font-size: 18px; - line-height: 22px; - display: inline-block; - font-weight: $gl-font-weight-normal; - color: $gl-text-color; vertical-align: top; white-space: nowrap; + img { + height: 28px; + margin-right: 8px; + } + &.wrap { white-space: normal; } @@ -259,30 +177,17 @@ header { } a { - color: currentColor; + display: -webkit-flex; + display: flex; + align-items: center; + padding: 2px 8px; + margin: 5px 2px 5px -8px; + border-radius: $border-radius-default; - &:hover { - text-decoration: underline; - color: $gl-header-nav-hover-color; - } - } - - .dropdown-toggle-caret { - color: $gl-text-color; - border: transparent; - background: transparent; - position: absolute; - top: 2px; - right: 3px; - width: 12px; - line-height: 19px; - padding: 0; - font-size: 10px; - text-align: center; - cursor: pointer; - - &:hover { - color: $gl-header-nav-hover-color; + svg { + @media (min-width: $screen-sm-min) { + margin-right: 8px; + } } } diff --git a/app/assets/stylesheets/framework/new-nav.scss b/app/assets/stylesheets/framework/new-nav.scss index 0ee07b18b1f..6120639f67f 100644 --- a/app/assets/stylesheets/framework/new-nav.scss +++ b/app/assets/stylesheets/framework/new-nav.scss @@ -1,67 +1,9 @@ .content-wrapper.page-with-new-nav { - margin-top: $new-navbar-height; + margin-top: $header-height; } -header.navbar-gitlab-new { - color: $white-light; - border-bottom: 0; - min-height: $new-navbar-height; - - .logo-text { - line-height: initial; - - svg { - width: 55px; - height: 14px; - margin: 0; - fill: $white-light; - } - } - +header { .header-content { - display: -webkit-flex; - display: flex; - padding-left: 0; - min-height: $new-navbar-height; - - .title-container { - display: -webkit-flex; - display: flex; - -webkit-align-items: stretch; - align-items: stretch; - -webkit-flex: 1 1 auto; - flex: 1 1 auto; - padding-top: 0; - overflow: visible; - } - - .title { - display: -webkit-flex; - display: flex; - padding-right: 0; - color: currentColor; - - img { - height: 28px; - margin-right: 8px; - } - - a { - display: -webkit-flex; - display: flex; - align-items: center; - padding: 2px 8px; - margin: 5px 2px 5px -8px; - border-radius: $border-radius-default; - - svg { - @media (min-width: $screen-sm-min) { - margin-right: 8px; - } - } - } - } - .dropdown.open { > a { border-bottom-color: $white-light; @@ -113,28 +55,6 @@ header.navbar-gitlab-new { } .container-fluid { - .navbar-toggle { - min-width: 45px; - padding: 0 $gl-padding; - margin-right: -7px; - text-align: center; - color: currentColor; - - svg { - fill: currentColor; - } - - &:hover, - &:focus, - &.active { - color: currentColor; - background-color: transparent; - - svg { - fill: currentColor; - } - } - } .navbar-nav { @media (max-width: $screen-xs-max) { @@ -242,7 +162,7 @@ header.navbar-gitlab-new { } } -.navbar-gitlab-new { +header { .navbar-sub-nav, .navbar-nav { > li { diff --git a/app/assets/stylesheets/framework/new-sidebar.scss b/app/assets/stylesheets/framework/new-sidebar.scss index 8332cec2962..caf4c7a40b1 100644 --- a/app/assets/stylesheets/framework/new-sidebar.scss +++ b/app/assets/stylesheets/framework/new-sidebar.scss @@ -24,7 +24,7 @@ $new-sidebar-collapsed-width: 50px; // Override position: absolute .right-sidebar { position: fixed; - height: calc(100% - #{$new-navbar-height}); + height: calc(100% - #{$header-height}); } .issues-bulk-update.right-sidebar.right-sidebar-expanded .issuable-sidebar-header { @@ -87,7 +87,7 @@ $new-sidebar-collapsed-width: 50px; z-index: 400; width: $new-sidebar-width; transition: left $sidebar-transition-duration; - top: $new-navbar-height; + top: $header-height; bottom: 0; left: 0; background-color: $gray-normal; @@ -197,7 +197,7 @@ $new-sidebar-collapsed-width: 50px; } .with-performance-bar .nav-sidebar { - top: $new-navbar-height + $performance-bar-height; + top: $header-height + $performance-bar-height; } .sidebar-sub-level-items { @@ -495,7 +495,7 @@ $new-sidebar-collapsed-width: 50px; // Make issue boards full-height now that sub-nav is gone .boards-list { - height: calc(100vh - #{$new-navbar-height}); + height: calc(100vh - #{$header-height}); @media (min-width: $screen-sm-min) { height: 475px; // Needed for PhantomJS @@ -506,5 +506,5 @@ $new-sidebar-collapsed-width: 50px; } .with-performance-bar .boards-list { - height: calc(100vh - #{$new-navbar-height} - #{$performance-bar-height}); + height: calc(100vh - #{$header-height} - #{$performance-bar-height}); } diff --git a/app/assets/stylesheets/framework/sidebar.scss b/app/assets/stylesheets/framework/sidebar.scss index 48dc25d343b..ef58382ba41 100644 --- a/app/assets/stylesheets/framework/sidebar.scss +++ b/app/assets/stylesheets/framework/sidebar.scss @@ -78,16 +78,16 @@ .right-sidebar { border-left: 1px solid $border-color; - height: calc(100% - #{$new-navbar-height}); + height: calc(100% - #{$header-height}); &.affix { position: fixed; - top: $new-navbar-height; + top: $header-height; } } .with-performance-bar .right-sidebar.affix { - top: $new-navbar-height + $performance-bar-height; + top: $header-height + $performance-bar-height; } @mixin maintain-sidebar-dimensions { diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index 5ab40947da9..dd3038c0252 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -224,8 +224,7 @@ $gl-sidebar-padding: 22px; $row-hover: $blue-50; $row-hover-border: $blue-200; $progress-color: #c0392b; -$header-height: 50px; -$new-navbar-height: 40px; +$header-height: 40px; $fixed-layout-width: 1280px; $limited-layout-width: 990px; $limited-layout-width-sm: 790px; diff --git a/app/assets/stylesheets/pages/builds.scss b/app/assets/stylesheets/pages/builds.scss index 359dd388d05..50ec5110bf1 100644 --- a/app/assets/stylesheets/pages/builds.scss +++ b/app/assets/stylesheets/pages/builds.scss @@ -64,10 +64,10 @@ color: $gl-text-color; position: sticky; position: -webkit-sticky; - top: $new-navbar-height; + top: $header-height; &.affix { - top: $new-navbar-height; + top: $header-height; } // with sidebar @@ -174,10 +174,10 @@ .with-performance-bar .build-page { .top-bar { - top: $new-navbar-height + $performance-bar-height; + top: $header-height + $performance-bar-height; &.affix { - top: $new-navbar-height + $performance-bar-height; + top: $header-height + $performance-bar-height; } } } diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index db3b7e89d7b..dae3ec7ac42 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -222,7 +222,7 @@ .right-sidebar { position: absolute; - top: $new-navbar-height; + top: $header-height; bottom: 0; right: 0; transition: width $right-sidebar-transition-duration; @@ -487,10 +487,10 @@ } .with-performance-bar .right-sidebar { - top: $new-navbar-height + $performance-bar-height; + top: $header-height + $performance-bar-height; .issuable-sidebar { - height: calc(100% - #{$new-navbar-height} - #{$performance-bar-height}); + height: calc(100% - #{$header-height} - #{$performance-bar-height}); } } diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 09a14578dd3..d9fb3b44d29 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -649,7 +649,7 @@ } .merge-request-tabs-holder { - top: $new-navbar-height; + top: $header-height; z-index: 200; background-color: $white-light; border-bottom: 1px solid $border-color; @@ -679,7 +679,7 @@ } .with-performance-bar .merge-request-tabs-holder { - top: $new-navbar-height + $performance-bar-height; + top: $header-height + $performance-bar-height; } .merge-request-tabs { diff --git a/app/views/layouts/header/_default.html.haml b/app/views/layouts/header/_default.html.haml index 7e9b76da570..3fbe318568e 100644 --- a/app/views/layouts/header/_default.html.haml +++ b/app/views/layouts/header/_default.html.haml @@ -1,4 +1,4 @@ -%header.navbar.navbar-gitlab.navbar-gitlab-new +%header.navbar.navbar-gitlab %a.sr-only.gl-accessibility{ href: "#content-body", tabindex: "1" } Skip to content .container-fluid .header-content -- cgit v1.2.1 From ac4e3cfee3e0f060105788db3f44aaf1ba821421 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Mon, 2 Oct 2017 07:57:15 -0700 Subject: Conflict fixes --- app/assets/stylesheets/framework/header.scss | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/assets/stylesheets/framework/header.scss b/app/assets/stylesheets/framework/header.scss index c10e3467bef..32cf2f6aa3e 100644 --- a/app/assets/stylesheets/framework/header.scss +++ b/app/assets/stylesheets/framework/header.scss @@ -57,7 +57,7 @@ header { .navbar-toggle { min-width: 45px; - padding: 4px $gl-padding; + padding: 0 $gl-padding; margin-right: -7px; font-size: 14px; text-align: center; @@ -73,16 +73,16 @@ header { } } - .fa-times { + .close-icon { display: none; } .menu-expanded { - .fa-ellipsis-v { + .more-icon { display: none; } - .fa-times { + .close-icon { display: block; } } -- cgit v1.2.1 From e0f6525efed763e87ecf12e53076f636f4c0765b Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Mon, 2 Oct 2017 08:32:13 -0700 Subject: Fix specs --- app/assets/stylesheets/framework/header.scss | 2 ++ app/assets/stylesheets/framework/new-nav.scss | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/app/assets/stylesheets/framework/header.scss b/app/assets/stylesheets/framework/header.scss index 32cf2f6aa3e..cd71d3c1dd9 100644 --- a/app/assets/stylesheets/framework/header.scss +++ b/app/assets/stylesheets/framework/header.scss @@ -103,6 +103,7 @@ header { } .header-content { + display: -webkit-flex; display: flex; position: relative; min-height: $header-height; @@ -143,6 +144,7 @@ header { } .title-container { + display: -webkit-flex; display: flex; -webkit-align-items: stretch; align-items: stretch; diff --git a/app/assets/stylesheets/framework/new-nav.scss b/app/assets/stylesheets/framework/new-nav.scss index 6120639f67f..a8fe8b31a6b 100644 --- a/app/assets/stylesheets/framework/new-nav.scss +++ b/app/assets/stylesheets/framework/new-nav.scss @@ -58,6 +58,7 @@ header { .navbar-nav { @media (max-width: $screen-xs-max) { + display: -webkit-flex; display: flex; padding-right: 10px; } @@ -178,6 +179,7 @@ header { } > a { + display: -webkit-flex; display: flex; align-items: center; justify-content: center; @@ -214,6 +216,7 @@ header { } .breadcrumbs { + display: -webkit-flex; display: flex; min-height: 48px; color: $gl-text-color; @@ -293,6 +296,7 @@ header { } .breadcrumbs-extra { + display: -webkit-flex; display: flex; flex: 0 0 auto; margin-left: auto; -- cgit v1.2.1 From bc45fd78df153bec48bc759cddc370fba934d447 Mon Sep 17 00:00:00 2001 From: kushalpandya Date: Fri, 6 Oct 2017 18:27:18 +0530 Subject: Remove unnecessary test --- spec/features/explore/new_menu_spec.rb | 6 ------ 1 file changed, 6 deletions(-) diff --git a/spec/features/explore/new_menu_spec.rb b/spec/features/explore/new_menu_spec.rb index e1c74a24890..c5ec495a418 100644 --- a/spec/features/explore/new_menu_spec.rb +++ b/spec/features/explore/new_menu_spec.rb @@ -128,12 +128,6 @@ feature 'Top Plus Menu', :js do expect(find('.header-new.dropdown')).not_to have_selector('.header-new-project-snippet') end - scenario 'public project has no New Issue Button' do - visit project_path(public_project) - - hasnot_topmenuitem("New issue") - end - scenario 'public project has no New merge request menu item' do visit project_path(public_project) -- cgit v1.2.1 From 55e6552c8d28d646a4b83777e8cb8f60681efc27 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Fri, 6 Oct 2017 19:47:00 +0000 Subject: Fix LDAP config key name --- doc/administration/auth/ldap.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/administration/auth/ldap.md b/doc/administration/auth/ldap.md index ad904908472..ad903aef896 100644 --- a/doc/administration/auth/ldap.md +++ b/doc/administration/auth/ldap.md @@ -287,11 +287,11 @@ LDAP email address, and then sign into GitLab via their LDAP credentials. There are two encryption methods, `simple_tls` and `start_tls`. -For either encryption method, if setting `validate_certificates: false`, TLS +For either encryption method, if setting `verify_certificates: false`, TLS encryption is established with the LDAP server before any LDAP-protocol data is exchanged but no validation of the LDAP server's SSL certificate is performed. ->**Note**: Before GitLab 9.5, `validate_certificates: false` is the default if +>**Note**: Before GitLab 9.5, `verify_certificates: false` is the default if unspecified. ## Limitations -- cgit v1.2.1 From e3fe48b85dc07b135180f8ec54db95537f794fdc Mon Sep 17 00:00:00 2001 From: Mike Greiling Date: Fri, 6 Oct 2017 15:29:32 -0500 Subject: allow prometheus graphs to handle non-congiuous paths (with NaN, +Inf, and -Inf) --- app/assets/javascripts/monitoring/utils/multiple_time_series.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/assets/javascripts/monitoring/utils/multiple_time_series.js b/app/assets/javascripts/monitoring/utils/multiple_time_series.js index 3cbe06d8fd6..65eec0d8d02 100644 --- a/app/assets/javascripts/monitoring/utils/multiple_time_series.js +++ b/app/assets/javascripts/monitoring/utils/multiple_time_series.js @@ -56,12 +56,16 @@ export default function createTimeSeries(queryData, graphWidth, graphHeight, gra timeSeriesScaleX.ticks(d3.time.minute, 60); timeSeriesScaleY.domain([0, maxValueFromSeries.maxValue]); + const defined = d => !isNaN(d.value) && d.value != null; + const lineFunction = d3.svg.line() + .defined(defined) .interpolate('linear') .x(d => timeSeriesScaleX(d.time)) .y(d => timeSeriesScaleY(d.value)); const areaFunction = d3.svg.area() + .defined(defined) .interpolate('linear') .x(d => timeSeriesScaleX(d.time)) .y0(graphHeight - graphHeightOffset) -- cgit v1.2.1 From ece9621b1304c7ebc338a6765c7e9aed5126edaa Mon Sep 17 00:00:00 2001 From: Mike Greiling Date: Fri, 6 Oct 2017 15:30:09 -0500 Subject: allow legend to properly handle NaN values --- app/assets/javascripts/monitoring/components/graph/legend.vue | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/monitoring/components/graph/legend.vue b/app/assets/javascripts/monitoring/components/graph/legend.vue index dbc48c63747..85b6d7f4cbe 100644 --- a/app/assets/javascripts/monitoring/components/graph/legend.vue +++ b/app/assets/javascripts/monitoring/components/graph/legend.vue @@ -79,7 +79,11 @@ }, formatMetricUsage(series) { - return `${formatRelevantDigits(series.values[this.currentDataIndex].value)} ${this.unitOfDisplay}`; + const value = series.values[this.currentDataIndex].value; + if (isNaN(value)) { + return '-'; + } + return `${formatRelevantDigits(value)} ${this.unitOfDisplay}`; }, createSeriesString(index, series) { -- cgit v1.2.1 From d13669716ab0c31ce9039ae9f7f073e33a4dc40f Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Tue, 19 Sep 2017 09:44:58 +0200 Subject: Create idea of read-only database In GitLab EE, a GitLab instance can be read-only (e.g. when it's a Geo secondary node). But in GitLab CE it also might be useful to have the "read-only" idea around. So port it back to GitLab CE. Also having the principle of read-only in GitLab CE would hopefully lead to less errors introduced, doing write operations when there aren't allowed for read-only calls. Closes gitlab-org/gitlab-ce#37534. --- app/controllers/admin/application_controller.rb | 14 ++ app/controllers/boards/issues_controller.rb | 2 +- app/controllers/projects/lfs_api_controller.rb | 18 +++ .../merge_requests/application_controller.rb | 2 +- app/controllers/sessions_controller.rb | 37 +++--- app/models/concerns/cache_markdown_field.rb | 14 +- app/models/concerns/routable.rb | 2 + app/models/concerns/token_authenticatable.rb | 4 +- app/models/merge_request.rb | 2 +- app/models/project.rb | 4 +- app/models/user.rb | 8 ++ app/services/keys/last_used_service.rb | 2 + app/services/users/activity_service.rb | 2 +- changelogs/unreleased/tc-geo-read-only-idea.yml | 5 + config/application.rb | 3 + doc/development/verifying_database_capabilities.md | 12 ++ lib/banzai/renderer.rb | 7 +- lib/gitlab/database.rb | 9 ++ lib/gitlab/git_access.rb | 9 +- lib/gitlab/git_access_wiki.rb | 5 + lib/gitlab/middleware/read_only.rb | 88 +++++++++++++ .../app/git_user_default_ssh_config_check.rb | 4 +- spec/factories/projects.rb | 2 +- spec/lib/banzai/renderer_spec.rb | 9 +- spec/lib/gitlab/git_access_spec.rb | 23 ++++ spec/lib/gitlab/git_access_wiki_spec.rb | 29 +++-- spec/lib/gitlab/middleware/read_only_spec.rb | 142 +++++++++++++++++++++ .../app/git_user_default_ssh_config_check_spec.rb | 8 ++ spec/models/concerns/cache_markdown_field_spec.rb | 72 ++++++----- spec/models/concerns/routable_spec.rb | 10 ++ spec/models/project_spec.rb | 42 +++++- spec/requests/lfs_http_spec.rb | 28 ++++ .../hashed_storage_migration_service_spec.rb | 2 +- spec/services/users/activity_service_spec.rb | 12 ++ 34 files changed, 546 insertions(+), 86 deletions(-) create mode 100644 changelogs/unreleased/tc-geo-read-only-idea.yml create mode 100644 lib/gitlab/middleware/read_only.rb create mode 100644 spec/lib/gitlab/middleware/read_only_spec.rb diff --git a/app/controllers/admin/application_controller.rb b/app/controllers/admin/application_controller.rb index a4648b33cfa..c27f2ee3c09 100644 --- a/app/controllers/admin/application_controller.rb +++ b/app/controllers/admin/application_controller.rb @@ -3,9 +3,23 @@ # Automatically sets the layout and ensures an administrator is logged in class Admin::ApplicationController < ApplicationController before_action :authenticate_admin! + before_action :display_read_only_information layout 'admin' def authenticate_admin! render_404 unless current_user.admin? end + + def display_read_only_information + return unless Gitlab::Database.read_only? + + flash.now[:notice] = read_only_message + end + + private + + # Overridden in EE + def read_only_message + _('You are on a read-only GitLab instance.') + end end diff --git a/app/controllers/boards/issues_controller.rb b/app/controllers/boards/issues_controller.rb index 0d74078645a..737656b3dcc 100644 --- a/app/controllers/boards/issues_controller.rb +++ b/app/controllers/boards/issues_controller.rb @@ -10,7 +10,7 @@ module Boards def index issues = Boards::Issues::ListService.new(board_parent, current_user, filter_params).execute issues = issues.page(params[:page]).per(params[:per] || 20) - make_sure_position_is_set(issues) + make_sure_position_is_set(issues) if Gitlab::Database.read_write? issues = issues.preload(:project, :milestone, :assignees, diff --git a/app/controllers/projects/lfs_api_controller.rb b/app/controllers/projects/lfs_api_controller.rb index 1b0d3aab3fa..536f908d2c5 100644 --- a/app/controllers/projects/lfs_api_controller.rb +++ b/app/controllers/projects/lfs_api_controller.rb @@ -2,6 +2,7 @@ class Projects::LfsApiController < Projects::GitHttpClientController include LfsRequest skip_before_action :lfs_check_access!, only: [:deprecated] + before_action :lfs_check_batch_operation!, only: [:batch] def batch unless objects.present? @@ -90,4 +91,21 @@ class Projects::LfsApiController < Projects::GitHttpClientController } } end + + def lfs_check_batch_operation! + if upload_request? && Gitlab::Database.read_only? + render( + json: { + message: lfs_read_only_message + }, + content_type: 'application/vnd.git-lfs+json', + status: 403 + ) + end + end + + # Overridden in EE + def lfs_read_only_message + _('You cannot write to this read-only GitLab instance.') + end end diff --git a/app/controllers/projects/merge_requests/application_controller.rb b/app/controllers/projects/merge_requests/application_controller.rb index eb7d7bf374c..0e71977a58a 100644 --- a/app/controllers/projects/merge_requests/application_controller.rb +++ b/app/controllers/projects/merge_requests/application_controller.rb @@ -13,7 +13,7 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont # Make sure merge requests created before 8.0 # have head file in refs/merge-requests/ def ensure_ref_fetched - @merge_request.ensure_ref_fetched + @merge_request.ensure_ref_fetched if Gitlab::Database.read_write? end def merge_request_params diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index ada91694fd6..c01be42c3ee 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -8,8 +8,7 @@ class SessionsController < Devise::SessionsController prepend_before_action :check_initial_setup, only: [:new] prepend_before_action :authenticate_with_two_factor, if: :two_factor_enabled?, only: [:create] - prepend_before_action :store_redirect_path, only: [:new] - + prepend_before_action :store_redirect_uri, only: [:new] before_action :auto_sign_in_with_provider, only: [:new] before_action :load_recaptcha @@ -86,28 +85,36 @@ class SessionsController < Devise::SessionsController end end - def store_redirect_path - redirect_path = + def stored_redirect_uri + @redirect_to ||= stored_location_for(:redirect) + end + + def store_redirect_uri + redirect_uri = if request.referer.present? && (params['redirect_to_referer'] == 'yes') - referer_uri = URI(request.referer) - if referer_uri.host == Gitlab.config.gitlab.host - referer_uri.request_uri - else - request.fullpath - end + URI(request.referer) else - request.fullpath + URI(request.url) end # Prevent a 'you are already signed in' message directly after signing: # we should never redirect to '/users/sign_in' after signing in successfully. - unless URI(redirect_path).path == new_user_session_path - store_location_for(:redirect, redirect_path) - end + return true if redirect_uri.path == new_user_session_path + + redirect_to = redirect_uri.to_s if redirect_allowed_to?(redirect_uri) + + @redirect_to = redirect_to + store_location_for(:redirect, redirect_to) + end + + # Overridden in EE + def redirect_allowed_to?(uri) + uri.host == Gitlab.config.gitlab.host && + uri.port == Gitlab.config.gitlab.port end def two_factor_enabled? - find_user.try(:two_factor_enabled?) + find_user&.two_factor_enabled? end def auto_sign_in_with_provider diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb index 193e459977a..9417033d1f6 100644 --- a/app/models/concerns/cache_markdown_field.rb +++ b/app/models/concerns/cache_markdown_field.rb @@ -59,7 +59,7 @@ module CacheMarkdownField # Update every column in a row if any one is invalidated, as we only store # one version per row - def refresh_markdown_cache!(do_update: false) + def refresh_markdown_cache options = { skip_project_check: skip_project_check? } updates = cached_markdown_fields.markdown_fields.map do |markdown_field| @@ -71,8 +71,14 @@ module CacheMarkdownField updates['cached_markdown_version'] = CacheMarkdownField::CACHE_VERSION updates.each {|html_field, data| write_attribute(html_field, data) } + end + + def refresh_markdown_cache! + updates = refresh_markdown_cache + + return unless persisted? && Gitlab::Database.read_write? - update_columns(updates) if persisted? && do_update + update_columns(updates) end def cached_html_up_to_date?(markdown_field) @@ -124,8 +130,8 @@ module CacheMarkdownField end # Using before_update here conflicts with elasticsearch-model somehow - before_create :refresh_markdown_cache!, if: :invalidated_markdown_cache? - before_update :refresh_markdown_cache!, if: :invalidated_markdown_cache? + before_create :refresh_markdown_cache, if: :invalidated_markdown_cache? + before_update :refresh_markdown_cache, if: :invalidated_markdown_cache? end class_methods do diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb index 12e93be2104..22fde2eb134 100644 --- a/app/models/concerns/routable.rb +++ b/app/models/concerns/routable.rb @@ -156,6 +156,8 @@ module Routable end def update_route + return if Gitlab::Database.read_only? + prepare_route route.save end diff --git a/app/models/concerns/token_authenticatable.rb b/app/models/concerns/token_authenticatable.rb index a7d5de48c66..ec3543f7053 100644 --- a/app/models/concerns/token_authenticatable.rb +++ b/app/models/concerns/token_authenticatable.rb @@ -43,15 +43,17 @@ module TokenAuthenticatable write_attribute(token_field, token) if token end + # Returns a token, but only saves when the database is in read & write mode define_method("ensure_#{token_field}!") do send("reset_#{token_field}!") if read_attribute(token_field).blank? # rubocop:disable GitlabSecurity/PublicSend read_attribute(token_field) end + # Resets the token, but only saves when the database is in read & write mode define_method("reset_#{token_field}!") do write_new_token(token_field) - save! + save! if Gitlab::Database.read_write? end end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 086226618e6..992cf63b704 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -477,7 +477,7 @@ class MergeRequest < ActiveRecord::Base end def check_if_can_be_merged - return unless unchecked? + return unless unchecked? && Gitlab::Database.read_write? can_be_merged = !broken? && project.repository.can_be_merged?(diff_head_sha, target_branch) diff --git a/app/models/project.rb b/app/models/project.rb index e51e70f01b7..523ac6cccd7 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -814,7 +814,7 @@ class Project < ActiveRecord::Base end def cache_has_external_issue_tracker - update_column(:has_external_issue_tracker, services.external_issue_trackers.any?) + update_column(:has_external_issue_tracker, services.external_issue_trackers.any?) if Gitlab::Database.read_write? end def has_wiki? @@ -834,7 +834,7 @@ class Project < ActiveRecord::Base end def cache_has_external_wiki - update_column(:has_external_wiki, services.external_wikis.any?) + update_column(:has_external_wiki, services.external_wikis.any?) if Gitlab::Database.read_write? end def find_or_initialize_services(exceptions: []) diff --git a/app/models/user.rb b/app/models/user.rb index 4ba9130a75a..7780afe5608 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -459,6 +459,14 @@ class User < ActiveRecord::Base reset_password_sent_at.present? && reset_password_sent_at >= 1.minute.ago end + def remember_me! + super if ::Gitlab::Database.read_write? + end + + def forget_me! + super if ::Gitlab::Database.read_write? + end + def disable_two_factor! transaction do update_attributes( diff --git a/app/services/keys/last_used_service.rb b/app/services/keys/last_used_service.rb index 066f3246158..dbd79f7da55 100644 --- a/app/services/keys/last_used_service.rb +++ b/app/services/keys/last_used_service.rb @@ -16,6 +16,8 @@ module Keys end def update? + return false if ::Gitlab::Database.read_only? + last_used = key.last_used_at return false if last_used && (Time.zone.now - last_used) <= TIMEOUT diff --git a/app/services/users/activity_service.rb b/app/services/users/activity_service.rb index ab532a1fdcf..5803404c3c8 100644 --- a/app/services/users/activity_service.rb +++ b/app/services/users/activity_service.rb @@ -14,7 +14,7 @@ module Users private def record_activity - Gitlab::UserActivities.record(@author.id) + Gitlab::UserActivities.record(@author.id) if Gitlab::Database.read_write? Rails.logger.debug("Recorded activity: #{@activity} for User ID: #{@author.id} (username: #{@author.username})") end diff --git a/changelogs/unreleased/tc-geo-read-only-idea.yml b/changelogs/unreleased/tc-geo-read-only-idea.yml new file mode 100644 index 00000000000..e1b52eef2ca --- /dev/null +++ b/changelogs/unreleased/tc-geo-read-only-idea.yml @@ -0,0 +1,5 @@ +--- +title: Create idea of read-only database +merge_request: 14688 +author: +type: changed diff --git a/config/application.rb b/config/application.rb index ca2ab83becc..31e91835b9e 100644 --- a/config/application.rb +++ b/config/application.rb @@ -154,6 +154,9 @@ module Gitlab ENV['GITLAB_PATH_OUTSIDE_HOOK'] = ENV['PATH'] ENV['GIT_TERMINAL_PROMPT'] = '0' + # Gitlab Read-only middleware support + config.middleware.insert_after ActionDispatch::Flash, 'Gitlab::Middleware::ReadOnly' + config.generators do |g| g.factory_girl false end diff --git a/doc/development/verifying_database_capabilities.md b/doc/development/verifying_database_capabilities.md index cc6d62957e3..ffdeff47d4a 100644 --- a/doc/development/verifying_database_capabilities.md +++ b/doc/development/verifying_database_capabilities.md @@ -24,3 +24,15 @@ else run_query end ``` + +# Read-only database + +The database can be used in read-only mode. In this case we have to +make sure all GET requests don't attempt any write operations to the +database. If one of those requests wants to write to the database, it needs +to be wrapped in a `Gitlab::Database.read_only?` or `Gitlab::Database.read_write?` +guard, to make sure it doesn't for read-only databases. + +We have a Rails Middleware that filters any potentially writing +operations (the CUD operations of CRUD) and prevent the user from trying +to update the database and getting a 500 error (see `Gitlab::Middleware::ReadOnly`). diff --git a/lib/banzai/renderer.rb b/lib/banzai/renderer.rb index ceca9296851..5f91884a878 100644 --- a/lib/banzai/renderer.rb +++ b/lib/banzai/renderer.rb @@ -40,7 +40,7 @@ module Banzai return cacheless_render_field(object, field) end - object.refresh_markdown_cache!(do_update: update_object?(object)) unless object.cached_html_up_to_date?(field) + object.refresh_markdown_cache! unless object.cached_html_up_to_date?(field) object.cached_html_for(field) end @@ -162,10 +162,5 @@ module Banzai return unless cache_key Rails.cache.__send__(:expanded_key, full_cache_key(cache_key, pipeline_name)) # rubocop:disable GitlabSecurity/PublicSend end - - # GitLab EE needs to disable updates on GET requests in Geo - def self.update_object?(object) - true - end end end diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb index a6ec75da385..357f16936c6 100644 --- a/lib/gitlab/database.rb +++ b/lib/gitlab/database.rb @@ -29,6 +29,15 @@ module Gitlab adapter_name.casecmp('postgresql').zero? end + # Overridden in EE + def self.read_only? + false + end + + def self.read_write? + !self.read_only? + end + def self.version database_version.match(/\A(?:PostgreSQL |)([^\s]+).*\z/)[1] end diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index db67ede9d9e..42b59c106e2 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -17,7 +17,8 @@ module Gitlab command_not_allowed: "The command you're trying to execute is not allowed.", upload_pack_disabled_over_http: 'Pulling over HTTP is not allowed.', receive_pack_disabled_over_http: 'Pushing over HTTP is not allowed.', - readonly: 'The repository is temporarily read-only. Please try again later.' + read_only: 'The repository is temporarily read-only. Please try again later.', + cannot_push_to_read_only: "You can't push code to a read-only GitLab instance." }.freeze DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive }.freeze @@ -161,7 +162,11 @@ module Gitlab def check_push_access!(changes) if project.repository_read_only? - raise UnauthorizedError, ERROR_MESSAGES[:readonly] + raise UnauthorizedError, ERROR_MESSAGES[:read_only] + end + + if Gitlab::Database.read_only? + raise UnauthorizedError, ERROR_MESSAGES[:cannot_push_to_read_only] end if deploy_key diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb index 1fe5155c093..98f1f45b338 100644 --- a/lib/gitlab/git_access_wiki.rb +++ b/lib/gitlab/git_access_wiki.rb @@ -1,6 +1,7 @@ module Gitlab class GitAccessWiki < GitAccess ERROR_MESSAGES = { + read_only: "You can't push code to a read-only GitLab instance.", write_to_wiki: "You are not allowed to write to this project's wiki." }.freeze @@ -17,6 +18,10 @@ module Gitlab raise UnauthorizedError, ERROR_MESSAGES[:write_to_wiki] end + if Gitlab::Database.read_only? + raise UnauthorizedError, ERROR_MESSAGES[:read_only] + end + true end end diff --git a/lib/gitlab/middleware/read_only.rb b/lib/gitlab/middleware/read_only.rb new file mode 100644 index 00000000000..0de0cddcce4 --- /dev/null +++ b/lib/gitlab/middleware/read_only.rb @@ -0,0 +1,88 @@ +module Gitlab + module Middleware + class ReadOnly + DISALLOWED_METHODS = %w(POST PATCH PUT DELETE).freeze + APPLICATION_JSON = 'application/json'.freeze + API_VERSIONS = (3..4) + + def initialize(app) + @app = app + @whitelisted = internal_routes + end + + def call(env) + @env = env + + if disallowed_request? && Gitlab::Database.read_only? + Rails.logger.debug('GitLab ReadOnly: preventing possible non read-only operation') + error_message = 'You cannot do writing operations on a read-only GitLab instance' + + if json_request? + return [403, { 'Content-Type' => 'application/json' }, [{ 'message' => error_message }.to_json]] + else + rack_flash.alert = error_message + rack_session['flash'] = rack_flash.to_session_value + + return [301, { 'Location' => last_visited_url }, []] + end + end + + @app.call(env) + end + + private + + def internal_routes + API_VERSIONS.flat_map { |version| "api/v#{version}/internal" } + end + + def disallowed_request? + DISALLOWED_METHODS.include?(@env['REQUEST_METHOD']) && !whitelisted_routes + end + + def json_request? + request.media_type == APPLICATION_JSON + end + + def rack_flash + @rack_flash ||= ActionDispatch::Flash::FlashHash.from_session_value(rack_session) + end + + def rack_session + @env['rack.session'] + end + + def request + @env['rack.request'] ||= Rack::Request.new(@env) + end + + def last_visited_url + @env['HTTP_REFERER'] || rack_session['user_return_to'] || Rails.application.routes.url_helpers.root_url + end + + def route_hash + @route_hash ||= Rails.application.routes.recognize_path(request.url, { method: request.request_method }) rescue {} + end + + def whitelisted_routes + logout_route || grack_route || @whitelisted.any? { |path| request.path.include?(path) } || lfs_route || sidekiq_route + end + + def logout_route + route_hash[:controller] == 'sessions' && route_hash[:action] == 'destroy' + end + + def sidekiq_route + request.path.start_with?('/admin/sidekiq') + end + + def grack_route + request.path.end_with?('.git/git-upload-pack') + end + + def lfs_route + request.path.end_with?('/info/lfs/objects/batch') + end + end + end +end diff --git a/lib/system_check/app/git_user_default_ssh_config_check.rb b/lib/system_check/app/git_user_default_ssh_config_check.rb index dfa8b8b3f5b..9af21078403 100644 --- a/lib/system_check/app/git_user_default_ssh_config_check.rb +++ b/lib/system_check/app/git_user_default_ssh_config_check.rb @@ -11,10 +11,10 @@ module SystemCheck ].freeze set_name 'Git user has default SSH configuration?' - set_skip_reason 'skipped (git user is not present or configured)' + set_skip_reason 'skipped (GitLab read-only, or git user is not present / configured)' def skip? - !home_dir || !File.directory?(home_dir) + Gitlab::Database.read_only? || !home_dir || !File.directory?(home_dir) end def check? diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index 958d62181a2..4034e7905ad 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -149,7 +149,7 @@ FactoryGirl.define do end end - trait :readonly do + trait :read_only do repository_read_only true end diff --git a/spec/lib/banzai/renderer_spec.rb b/spec/lib/banzai/renderer_spec.rb index da42272bbef..81a04a2d46d 100644 --- a/spec/lib/banzai/renderer_spec.rb +++ b/spec/lib/banzai/renderer_spec.rb @@ -31,7 +31,14 @@ describe Banzai::Renderer do let(:object) { fake_object(fresh: false) } it 'caches and returns the result' do - expect(object).to receive(:refresh_markdown_cache!).with(do_update: true) + expect(object).to receive(:refresh_markdown_cache!) + + is_expected.to eq('field_html') + end + + it "skips database caching on a GitLab read-only instance" do + allow(Gitlab::Database).to receive(:read_only?).and_return(true) + expect(object).to receive(:refresh_markdown_cache!) is_expected.to eq('field_html') end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 458627ee4de..c54327bd2e4 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -598,6 +598,19 @@ describe Gitlab::GitAccess do admin: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false })) end end + + context "when in a read-only GitLab instance" do + before do + create(:protected_branch, name: 'feature', project: project) + allow(Gitlab::Database).to receive(:read_only?) { true } + end + + # Only check admin; if an admin can't do it, other roles can't either + matrix = permissions_matrix[:admin].dup + matrix.each { |key, _| matrix[key] = false } + + run_permission_checks(admin: matrix) + end end describe 'build authentication abilities' do @@ -632,6 +645,16 @@ describe Gitlab::GitAccess do end end + context 'when the repository is read only' do + let(:project) { create(:project, :repository, :read_only) } + + it 'denies push access' do + project.add_master(user) + + expect { push_access_check }.to raise_unauthorized('The repository is temporarily read-only. Please try again later.') + end + end + describe 'deploy key permissions' do let(:key) { create(:deploy_key, user: user, can_push: can_push) } let(:actor) { key } diff --git a/spec/lib/gitlab/git_access_wiki_spec.rb b/spec/lib/gitlab/git_access_wiki_spec.rb index 0376b4ee783..1056074264a 100644 --- a/spec/lib/gitlab/git_access_wiki_spec.rb +++ b/spec/lib/gitlab/git_access_wiki_spec.rb @@ -4,6 +4,7 @@ describe Gitlab::GitAccessWiki do let(:access) { described_class.new(user, project, 'web', authentication_abilities: authentication_abilities, redirected_path: redirected_path) } let(:project) { create(:project, :repository) } let(:user) { create(:user) } + let(:changes) { ['6f6d7e7ed 570e7b2ab refs/heads/master'] } let(:redirected_path) { nil } let(:authentication_abilities) do [ @@ -13,19 +14,27 @@ describe Gitlab::GitAccessWiki do ] end - describe 'push_allowed?' do - before do - create(:protected_branch, name: 'master', project: project) - project.team << [user, :developer] - end + describe '#push_access_check' do + context 'when user can :create_wiki' do + before do + create(:protected_branch, name: 'master', project: project) + project.team << [user, :developer] + end - subject { access.check('git-receive-pack', changes) } + subject { access.check('git-receive-pack', changes) } - it { expect { subject }.not_to raise_error } - end + it { expect { subject }.not_to raise_error } + + context 'when in a read-only GitLab instance' do + before do + allow(Gitlab::Database).to receive(:read_only?) { true } + end - def changes - ['6f6d7e7ed 570e7b2ab refs/heads/master'] + it 'does not give access to upload wiki code' do + expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, "You can't push code to a read-only GitLab instance.") + end + end + end end describe '#access_check_download!' do diff --git a/spec/lib/gitlab/middleware/read_only_spec.rb b/spec/lib/gitlab/middleware/read_only_spec.rb new file mode 100644 index 00000000000..742a792a1af --- /dev/null +++ b/spec/lib/gitlab/middleware/read_only_spec.rb @@ -0,0 +1,142 @@ +require 'spec_helper' + +describe Gitlab::Middleware::ReadOnly do + include Rack::Test::Methods + + RSpec::Matchers.define :be_a_redirect do + match do |response| + response.status == 301 + end + end + + RSpec::Matchers.define :disallow_request do + match do |middleware| + flash = middleware.send(:rack_flash) + flash['alert'] && flash['alert'].include?('You cannot do writing operations') + end + end + + RSpec::Matchers.define :disallow_request_in_json do + match do |response| + json_response = JSON.parse(response.body) + response.body.include?('You cannot do writing operations') && json_response.key?('message') + end + end + + let(:rack_stack) do + rack = Rack::Builder.new do + use ActionDispatch::Session::CacheStore + use ActionDispatch::Flash + use ActionDispatch::ParamsParser + end + + rack.run(subject) + rack.to_app + end + + subject { described_class.new(fake_app) } + + let(:request) { Rack::MockRequest.new(rack_stack) } + + context 'normal requests to a read-only Gitlab instance' do + let(:fake_app) { lambda { |env| [200, { 'Content-Type' => 'text/plain' }, ['OK']] } } + + before do + allow(Gitlab::Database).to receive(:read_only?) { true } + end + + it 'expects PATCH requests to be disallowed' do + response = request.patch('/test_request') + + expect(response).to be_a_redirect + expect(subject).to disallow_request + end + + it 'expects PUT requests to be disallowed' do + response = request.put('/test_request') + + expect(response).to be_a_redirect + expect(subject).to disallow_request + end + + it 'expects POST requests to be disallowed' do + response = request.post('/test_request') + + expect(response).to be_a_redirect + expect(subject).to disallow_request + end + + it 'expects a internal POST request to be allowed after a disallowed request' do + response = request.post('/test_request') + + expect(response).to be_a_redirect + + response = request.post("/api/#{API::API.version}/internal") + + expect(response).not_to be_a_redirect + end + + it 'expects DELETE requests to be disallowed' do + response = request.delete('/test_request') + + expect(response).to be_a_redirect + expect(subject).to disallow_request + end + + context 'whitelisted requests' do + it 'expects DELETE request to logout to be allowed' do + response = request.delete('/users/sign_out') + + expect(response).not_to be_a_redirect + expect(subject).not_to disallow_request + end + + it 'expects a POST internal request to be allowed' do + response = request.post("/api/#{API::API.version}/internal") + + expect(response).not_to be_a_redirect + expect(subject).not_to disallow_request + end + + it 'expects a POST LFS request to batch URL to be allowed' do + response = request.post('/root/rouge.git/info/lfs/objects/batch') + + expect(response).not_to be_a_redirect + expect(subject).not_to disallow_request + end + end + end + + context 'json requests to a read-only GitLab instance' do + let(:fake_app) { lambda { |env| [200, { 'Content-Type' => 'application/json' }, ['OK']] } } + let(:content_json) { { 'CONTENT_TYPE' => 'application/json' } } + + before do + allow(Gitlab::Database).to receive(:read_only?) { true } + end + + it 'expects PATCH requests to be disallowed' do + response = request.patch('/test_request', content_json) + + expect(response).to disallow_request_in_json + end + + it 'expects PUT requests to be disallowed' do + response = request.put('/test_request', content_json) + + expect(response).to disallow_request_in_json + end + + it 'expects POST requests to be disallowed' do + response = request.post('/test_request', content_json) + + expect(response).to disallow_request_in_json + end + + it 'expects DELETE requests to be disallowed' do + response = request.delete('/test_request', content_json) + + expect(response).to disallow_request_in_json + end + end +end diff --git a/spec/lib/system_check/app/git_user_default_ssh_config_check_spec.rb b/spec/lib/system_check/app/git_user_default_ssh_config_check_spec.rb index a0fb86345f3..b4b83b70d1c 100644 --- a/spec/lib/system_check/app/git_user_default_ssh_config_check_spec.rb +++ b/spec/lib/system_check/app/git_user_default_ssh_config_check_spec.rb @@ -39,6 +39,14 @@ describe SystemCheck::App::GitUserDefaultSSHConfigCheck do it { is_expected.to eq(expected_result) } end + + it 'skips GitLab read-only instances' do + stub_user + stub_home_dir + allow(Gitlab::Database).to receive(:read_only?).and_return(true) + + is_expected.to be_truthy + end end describe '#check?' do diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb index 40bbb10eaac..129dfa07f15 100644 --- a/spec/models/concerns/cache_markdown_field_spec.rb +++ b/spec/models/concerns/cache_markdown_field_spec.rb @@ -178,57 +178,59 @@ describe CacheMarkdownField do end end - describe '#refresh_markdown_cache!' do + describe '#refresh_markdown_cache' do before do thing.foo = updated_markdown end - context 'do_update: false' do - it 'fills all html fields' do - thing.refresh_markdown_cache! + it 'fills all html fields' do + thing.refresh_markdown_cache - expect(thing.foo_html).to eq(updated_html) - expect(thing.foo_html_changed?).to be_truthy - expect(thing.baz_html_changed?).to be_truthy - end + expect(thing.foo_html).to eq(updated_html) + expect(thing.foo_html_changed?).to be_truthy + expect(thing.baz_html_changed?).to be_truthy + end - it 'does not save the result' do - expect(thing).not_to receive(:update_columns) + it 'does not save the result' do + expect(thing).not_to receive(:update_columns) - thing.refresh_markdown_cache! - end + thing.refresh_markdown_cache + end - it 'updates the markdown cache version' do - thing.cached_markdown_version = nil - thing.refresh_markdown_cache! + it 'updates the markdown cache version' do + thing.cached_markdown_version = nil + thing.refresh_markdown_cache - expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_VERSION) - end + expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_VERSION) end + end - context 'do_update: true' do - it 'fills all html fields' do - thing.refresh_markdown_cache!(do_update: true) + describe '#refresh_markdown_cache!' do + before do + thing.foo = updated_markdown + end - expect(thing.foo_html).to eq(updated_html) - expect(thing.foo_html_changed?).to be_truthy - expect(thing.baz_html_changed?).to be_truthy - end + it 'fills all html fields' do + thing.refresh_markdown_cache! - it 'skips saving if not persisted' do - expect(thing).to receive(:persisted?).and_return(false) - expect(thing).not_to receive(:update_columns) + expect(thing.foo_html).to eq(updated_html) + expect(thing.foo_html_changed?).to be_truthy + expect(thing.baz_html_changed?).to be_truthy + end - thing.refresh_markdown_cache!(do_update: true) - end + it 'skips saving if not persisted' do + expect(thing).to receive(:persisted?).and_return(false) + expect(thing).not_to receive(:update_columns) - it 'saves the changes using #update_columns' do - expect(thing).to receive(:persisted?).and_return(true) - expect(thing).to receive(:update_columns) - .with("foo_html" => updated_html, "baz_html" => "", "cached_markdown_version" => CacheMarkdownField::CACHE_VERSION) + thing.refresh_markdown_cache! + end - thing.refresh_markdown_cache!(do_update: true) - end + it 'saves the changes using #update_columns' do + expect(thing).to receive(:persisted?).and_return(true) + expect(thing).to receive(:update_columns) + .with("foo_html" => updated_html, "baz_html" => "", "cached_markdown_version" => CacheMarkdownField::CACHE_VERSION) + + thing.refresh_markdown_cache! end end diff --git a/spec/models/concerns/routable_spec.rb b/spec/models/concerns/routable_spec.rb index b463d12e448..ab8773b7ede 100644 --- a/spec/models/concerns/routable_spec.rb +++ b/spec/models/concerns/routable_spec.rb @@ -12,6 +12,16 @@ describe Group, 'Routable' do it { is_expected.to have_many(:redirect_routes).dependent(:destroy) } end + describe 'GitLab read-only instance' do + it 'does not save route if route is not present' do + group.route.path = '' + allow(Gitlab::Database).to receive(:read_only?).and_return(true) + expect(group).to receive(:update_route).and_call_original + + expect { group.full_path }.to change { Route.count }.by(0) + end + end + describe 'Callbacks' do it 'creates route record on create' do expect(group.route.path).to eq(group.path) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 9b470e79a76..ec700215521 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -692,6 +692,44 @@ describe Project do project.cache_has_external_issue_tracker end.to change { project.has_external_issue_tracker}.to(false) end + + it 'does not cache data when in a read-only GitLab instance' do + allow(Gitlab::Database).to receive(:read_only?) { true } + + expect do + project.cache_has_external_issue_tracker + end.not_to change { project.has_external_issue_tracker } + end + end + + describe '#cache_has_external_wiki' do + let(:project) { create(:project, has_external_wiki: nil) } + + it 'stores true if there is any external_wikis' do + services = double(:service, external_wikis: [ExternalWikiService.new]) + expect(project).to receive(:services).and_return(services) + + expect do + project.cache_has_external_wiki + end.to change { project.has_external_wiki}.to(true) + end + + it 'stores false if there is no external_wikis' do + services = double(:service, external_wikis: []) + expect(project).to receive(:services).and_return(services) + + expect do + project.cache_has_external_wiki + end.to change { project.has_external_wiki}.to(false) + end + + it 'does not cache data when in a read-only GitLab instance' do + allow(Gitlab::Database).to receive(:read_only?) { true } + + expect do + project.cache_has_external_wiki + end.not_to change { project.has_external_wiki } + end end describe '#has_wiki?' do @@ -2446,7 +2484,7 @@ describe Project do expect(project.migrate_to_hashed_storage!).to be_truthy end - it 'flags as readonly' do + it 'flags as read-only' do expect { project.migrate_to_hashed_storage! }.to change { project.repository_read_only }.to(true) end @@ -2573,7 +2611,7 @@ describe Project do expect(project.migrate_to_hashed_storage!).to be_nil end - it 'does not flag as readonly' do + it 'does not flag as read-only' do expect { project.migrate_to_hashed_storage! }.not_to change { project.repository_read_only } end end diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb index 27d09b8202e..eb4017e116b 100644 --- a/spec/requests/lfs_http_spec.rb +++ b/spec/requests/lfs_http_spec.rb @@ -824,6 +824,34 @@ describe 'Git LFS API and storage' do end end + describe 'when handling lfs batch request on a read-only GitLab instance' do + let(:authorization) { authorize_user } + let(:project) { create(:project) } + let(:path) { "#{project.http_url_to_repo}/info/lfs/objects/batch" } + let(:body) do + { 'objects' => [{ 'oid' => sample_oid, 'size' => sample_size }] } + end + + before do + allow(Gitlab::Database).to receive(:read_only?) { true } + project.team << [user, :master] + enable_lfs + end + + it 'responds with a 200 message on download' do + post_lfs_json path, body.merge('operation' => 'download'), headers + + expect(response).to have_gitlab_http_status(200) + end + + it 'responds with a 403 message on upload' do + post_lfs_json path, body.merge('operation' => 'upload'), headers + + expect(response).to have_gitlab_http_status(403) + expect(json_response).to include('message' => 'You cannot write to this read-only GitLab instance.') + end + end + describe 'when pushing a lfs object' do before do enable_lfs diff --git a/spec/services/projects/hashed_storage_migration_service_spec.rb b/spec/services/projects/hashed_storage_migration_service_spec.rb index 1b61207b550..aa1988d29d6 100644 --- a/spec/services/projects/hashed_storage_migration_service_spec.rb +++ b/spec/services/projects/hashed_storage_migration_service_spec.rb @@ -20,7 +20,7 @@ describe Projects::HashedStorageMigrationService do expect(gitlab_shell.exists?(project.repository_storage_path, "#{hashed_storage.disk_path}.wiki.git")).to be_truthy end - it 'updates project to be hashed and not readonly' do + it 'updates project to be hashed and not read-only' do service.execute expect(project.hashed_storage?).to be_truthy diff --git a/spec/services/users/activity_service_spec.rb b/spec/services/users/activity_service_spec.rb index fef4da0c76e..17eabad73be 100644 --- a/spec/services/users/activity_service_spec.rb +++ b/spec/services/users/activity_service_spec.rb @@ -38,6 +38,18 @@ describe Users::ActivityService do end end end + + context 'when in GitLab read-only instance' do + before do + allow(Gitlab::Database).to receive(:read_only?).and_return(true) + end + + it 'does not update last_activity_at' do + service.execute + + expect(last_hour_user_ids).to eq([]) + end + end end def last_hour_user_ids -- cgit v1.2.1 From 265b1a3b72ff7552e2ae01a059f80bd59714649d Mon Sep 17 00:00:00 2001 From: Winnie Hellmann Date: Fri, 6 Oct 2017 20:40:41 +0000 Subject: Show confirmation modal before deleting account --- app/assets/javascripts/lib/utils/csrf.js | 4 +- .../account/components/delete_account_modal.vue | 146 +++++++++++++++++++++ app/assets/javascripts/profile/account/index.js | 21 +++ app/assets/javascripts/repo/components/repo.vue | 2 +- .../vue_shared/components/popup_dialog.vue | 6 +- app/assets/stylesheets/framework/modal.scss | 13 +- app/assets/stylesheets/framework/variables.scss | 1 + app/controllers/registrations_controller.rb | 29 +++- app/models/user.rb | 4 + app/views/profiles/accounts/show.html.haml | 20 ++- .../unreleased/winh-delete-account-modal.yml | 5 + config/webpack.config.js | 1 + spec/controllers/registrations_controller_spec.rb | 64 ++++++++- spec/features/issues/issue_detail_spec.rb | 3 +- spec/features/profile_spec.rb | 44 ++++++- .../components/delete_account_modal_spec.js | 129 ++++++++++++++++++ spec/models/user_spec.rb | 45 +++++++ 17 files changed, 507 insertions(+), 30 deletions(-) create mode 100644 app/assets/javascripts/profile/account/components/delete_account_modal.vue create mode 100644 app/assets/javascripts/profile/account/index.js create mode 100644 changelogs/unreleased/winh-delete-account-modal.yml create mode 100644 spec/javascripts/profile/account/components/delete_account_modal_spec.js diff --git a/app/assets/javascripts/lib/utils/csrf.js b/app/assets/javascripts/lib/utils/csrf.js index ae41cc5e8a8..0bdb547d31a 100644 --- a/app/assets/javascripts/lib/utils/csrf.js +++ b/app/assets/javascripts/lib/utils/csrf.js @@ -14,6 +14,9 @@ If you need to compose a headers object, use the spread operator: someOtherHeader: '12345', } ``` + +see also http://guides.rubyonrails.org/security.html#cross-site-request-forgery-csrf +and https://github.com/rails/jquery-rails/blob/v4.3.1/vendor/assets/javascripts/jquery_ujs.js#L59-L62 */ const csrf = { @@ -53,4 +56,3 @@ if ($.rails) { } export default csrf; - diff --git a/app/assets/javascripts/profile/account/components/delete_account_modal.vue b/app/assets/javascripts/profile/account/components/delete_account_modal.vue new file mode 100644 index 00000000000..b2b34cb83e1 --- /dev/null +++ b/app/assets/javascripts/profile/account/components/delete_account_modal.vue @@ -0,0 +1,146 @@ + + + diff --git a/app/assets/javascripts/profile/account/index.js b/app/assets/javascripts/profile/account/index.js new file mode 100644 index 00000000000..635056e0eeb --- /dev/null +++ b/app/assets/javascripts/profile/account/index.js @@ -0,0 +1,21 @@ +import Vue from 'vue'; + +import deleteAccountModal from './components/delete_account_modal.vue'; + +const deleteAccountModalEl = document.getElementById('delete-account-modal'); +// eslint-disable-next-line no-new +new Vue({ + el: deleteAccountModalEl, + components: { + deleteAccountModal, + }, + render(createElement) { + return createElement('delete-account-modal', { + props: { + actionUrl: deleteAccountModalEl.dataset.actionUrl, + confirmWithPassword: !!deleteAccountModalEl.dataset.confirmWithPassword, + username: deleteAccountModalEl.dataset.username, + }, + }); + }, +}); diff --git a/app/assets/javascripts/repo/components/repo.vue b/app/assets/javascripts/repo/components/repo.vue index d6c864cb976..cc60aa5939c 100644 --- a/app/assets/javascripts/repo/components/repo.vue +++ b/app/assets/javascripts/repo/components/repo.vue @@ -62,7 +62,7 @@ export default { :primary-button-label="__('Discard changes')" kind="warning" :title="__('Are you sure?')" - :body="__('Are you sure you want to discard your changes?')" + :text="__('Are you sure you want to discard your changes?')" @toggle="toggleDialogOpen" @submit="dialogSubmitted" /> diff --git a/app/assets/javascripts/vue_shared/components/popup_dialog.vue b/app/assets/javascripts/vue_shared/components/popup_dialog.vue index 994b33bc1c9..9279b50cd55 100644 --- a/app/assets/javascripts/vue_shared/components/popup_dialog.vue +++ b/app/assets/javascripts/vue_shared/components/popup_dialog.vue @@ -7,7 +7,7 @@ export default { type: String, required: true, }, - body: { + text: { type: String, required: true, }, @@ -63,7 +63,9 @@ export default {