From 6b0a43aff36f0bbb9050b3c04155a3ccd9c1a75b Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 13 Jan 2016 21:17:28 +0100 Subject: [PATCH] Improve readability of artifacts browser `Entry` related code --- .../projects/artifacts_controller.rb | 10 +- app/models/ci/build.rb | 4 +- app/views/projects/artifacts/browse.html.haml | 12 +-- features/steps/project/builds.rb | 2 +- lib/gitlab/ci/build/artifacts/metadata.rb | 5 +- .../artifacts/metadata/{path.rb => entry.rb} | 49 +++++----- .../metadata/{path_spec.rb => entry_spec.rb} | 92 ++++++++++++------- .../ci/build/artifacts/metadata_spec.rb | 6 +- 8 files changed, 102 insertions(+), 78 deletions(-) rename lib/gitlab/ci/build/artifacts/metadata/{path.rb => entry.rb} (69%) rename spec/lib/gitlab/ci/build/artifacts/metadata/{path_spec.rb => entry_spec.rb} (58%) diff --git a/app/controllers/projects/artifacts_controller.rb b/app/controllers/projects/artifacts_controller.rb index 00daac0cb30..dff0732bdfe 100644 --- a/app/controllers/projects/artifacts_controller.rb +++ b/app/controllers/projects/artifacts_controller.rb @@ -18,17 +18,17 @@ class Projects::ArtifactsController < Projects::ApplicationController return render_404 unless build.artifacts? directory = params[:path] ? "#{params[:path]}/" : '' - @path = build.artifacts_metadata_path(directory) + @entry = build.artifacts_metadata_entry(directory) - return render_404 unless @path.exists? + return render_404 unless @entry.exists? end def file - file_path = build.artifacts_metadata_path(params[:path]) + entry = build.artifacts_metadata_entry(params[:path]) - if file_path.exists? + if entry.exists? render json: { archive: build.artifacts_file.path, - path: Base64.encode64(file_path.path) } + entry: Base64.encode64(entry.path) } else render json: {}, status: 404 end diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index fef667f865e..6cc26abce66 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -347,8 +347,8 @@ module Ci artifacts? && artifacts_metadata.exists? end - def artifacts_metadata_path(path) - Gitlab::Ci::Build::Artifacts::Metadata.new(artifacts_metadata.path, path).to_path + def artifacts_metadata_entry(path) + Gitlab::Ci::Build::Artifacts::Metadata.new(artifacts_metadata.path, path).to_entry end private diff --git a/app/views/projects/artifacts/browse.html.haml b/app/views/projects/artifacts/browse.html.haml index fc161be2ffc..1add7ef6bfb 100644 --- a/app/views/projects/artifacts/browse.html.haml +++ b/app/views/projects/artifacts/browse.html.haml @@ -1,11 +1,11 @@ -- page_title "#{@build.name} (##{@build.id})", 'Build artifacts' -- header_title project_title(@project, "Build artifacts", namespace_project_build_path(@project.namespace, @project, @build)) +- page_title 'Artifacts', "#{@build.name} (##{@build.id})", 'Builds' += render 'projects/builds/header_title' #tree-holder.tree-holder .gray-content-block.top-block.clearfix .pull-right = link_to download_namespace_project_build_artifacts_path(@project.namespace, @project, @build), - class: 'btn btn-default', method: :get do + class: 'btn btn-default' do = icon('download') Download artifacts archive @@ -17,8 +17,8 @@ %th Name %th Size %th Download - = render partial: 'tree_directory', collection: @path.directories!, as: :directory - = render partial: 'tree_file', collection: @path.files, as: :file + = render partial: 'tree_directory', collection: @entry.directories(parent: true), as: :directory + = render partial: 'tree_file', collection: @entry.files, as: :file -- if @path.children.empty? +- if @entry.empty? .center Empty diff --git a/features/steps/project/builds.rb b/features/steps/project/builds.rb index 5702496ac84..28395281077 100644 --- a/features/steps/project/builds.rb +++ b/features/steps/project/builds.rb @@ -84,6 +84,6 @@ class Spinach::Features::ProjectBuilds < Spinach::FeatureSteps # this will be accelerated by Workhorse response_json = JSON.parse(page.body, symbolize_names: true) expect(response_json[:archive]).to end_with('build_artifacts.zip') - expect(response_json[:path]).to eq Base64.encode64('ci_artifacts.txt') + expect(response_json[:entry]).to eq Base64.encode64('ci_artifacts.txt') end end diff --git a/lib/gitlab/ci/build/artifacts/metadata.rb b/lib/gitlab/ci/build/artifacts/metadata.rb index e9ec8f1302c..bfdfc9a1d7d 100644 --- a/lib/gitlab/ci/build/artifacts/metadata.rb +++ b/lib/gitlab/ci/build/artifacts/metadata.rb @@ -34,8 +34,9 @@ module Gitlab end end - def to_path - Path.new(@path, *match!) + def to_entry + entires, metadata = match! + Entry.new(@path, entires, metadata) end private diff --git a/lib/gitlab/ci/build/artifacts/metadata/path.rb b/lib/gitlab/ci/build/artifacts/metadata/entry.rb similarity index 69% rename from lib/gitlab/ci/build/artifacts/metadata/path.rb rename to lib/gitlab/ci/build/artifacts/metadata/entry.rb index 6896aa936d5..2fb6c327729 100644 --- a/lib/gitlab/ci/build/artifacts/metadata/path.rb +++ b/lib/gitlab/ci/build/artifacts/metadata/entry.rb @@ -2,7 +2,7 @@ module Gitlab module Ci::Build::Artifacts class Metadata ## - # Class that represents a simplified path to a file or + # Class that represents an entry (path and metadata) to a file or # directory in GitLab CI Build Artifacts binary file / archive # # This is IO-operations safe class, that does similar job to @@ -10,13 +10,13 @@ module Gitlab # # This class is working only with UTF-8 encoded paths. # - class Path - attr_reader :path, :universe + class Entry + attr_reader :path, :entires attr_accessor :name - def initialize(path, universe, metadata = []) + def initialize(path, entires, metadata = []) @path = path.force_encoding('UTF-8') - @universe = universe + @entires = entires @metadata = metadata if path.include?("\0") @@ -46,11 +46,11 @@ module Gitlab end def basename - (directory? && !blank_node?) ? name + ::File::SEPARATOR : name + (directory? && !blank_node?) ? name + '/' : name end def name - @name || @path.split(::File::SEPARATOR).last.to_s + @name || @path.split('/').last.to_s end def children @@ -58,20 +58,17 @@ module Gitlab return @children if @children child_pattern = %r{^#{Regexp.escape(@path)}[^/]+/?$} - @children = select { |entry| entry =~ child_pattern } + @children = select_entires { |entry| entry =~ child_pattern } end - def directories + def directories(opts = {}) return [] unless directory? - children.select(&:directory?) - end - - def directories! - return directories unless has_parent? + dirs = children.select(&:directory?) + return dirs unless has_parent? && opts[:parent] dotted_parent = parent dotted_parent.name = '..' - directories.prepend(dotted_parent) + dirs.prepend(dotted_parent) end def files @@ -80,7 +77,7 @@ module Gitlab end def metadata - @index ||= @universe.index(@path) + @index ||= @entires.index(@path) @metadata[@index] || {} end @@ -88,20 +85,24 @@ module Gitlab @path.count('/') + (file? ? 1 : 0) end - def exists? - blank_node? || @universe.include?(@path) - end - def blank_node? @path.empty? # "" is considered to be './' end + def exists? + blank_node? || @entires.include?(@path) + end + + def empty? + children.empty? + end + def to_s @path end def ==(other) - @path == other.path && @universe == other.universe + @path == other.path && @entires == other.entires end def inspect @@ -111,11 +112,11 @@ module Gitlab private def new(path) - self.class.new(path, @universe, @metadata) + self.class.new(path, @entires, @metadata) end - def select - selected = @universe.select { |entry| yield entry } + def select_entires + selected = @entires.select { |entry| yield entry } selected.map { |path| new(path) } end end diff --git a/spec/lib/gitlab/ci/build/artifacts/metadata/path_spec.rb b/spec/lib/gitlab/ci/build/artifacts/metadata/entry_spec.rb similarity index 58% rename from spec/lib/gitlab/ci/build/artifacts/metadata/path_spec.rb rename to spec/lib/gitlab/ci/build/artifacts/metadata/entry_spec.rb index 3b254c3ce2f..a728227f87c 100644 --- a/spec/lib/gitlab/ci/build/artifacts/metadata/path_spec.rb +++ b/spec/lib/gitlab/ci/build/artifacts/metadata/entry_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' -describe Gitlab::Ci::Build::Artifacts::Metadata::Path do - let(:universe) do +describe Gitlab::Ci::Build::Artifacts::Metadata::Entry do + let(:entries) do ['path/', 'path/dir_1/', 'path/dir_1/file_1', @@ -21,7 +21,7 @@ describe Gitlab::Ci::Build::Artifacts::Metadata::Path do end def string_path(string_path) - described_class.new(string_path, universe) + described_class.new(string_path, entries) end describe '/file/with/absolute_path', path: '/file/with/absolute_path' do @@ -79,21 +79,38 @@ describe Gitlab::Ci::Build::Artifacts::Metadata::Path do end describe '#directories' do - subject { |example| path(example).directories } + context 'without options' do + subject { |example| path(example).directories } - it { is_expected.to all(be_directory) } - it { is_expected.to all(be_an_instance_of described_class) } - it { is_expected.to contain_exactly string_path('path/dir_1/subdir/') } - end + it { is_expected.to all(be_directory) } + it { is_expected.to all(be_an_instance_of described_class) } + it { is_expected.to contain_exactly string_path('path/dir_1/subdir/') } + end - describe '#directories!' do - subject { |example| path(example).directories! } + context 'with option parent: true' do + subject { |example| path(example).directories(parent: true) } - it { is_expected.to all(be_directory) } - it { is_expected.to all(be_an_instance_of described_class) } - it do - is_expected.to contain_exactly string_path('path/dir_1/subdir/'), - string_path('path/') + it { is_expected.to all(be_directory) } + it { is_expected.to all(be_an_instance_of described_class) } + it do + is_expected.to contain_exactly string_path('path/dir_1/subdir/'), + string_path('path/') + end + end + + describe '#nodes' do + subject { |example| path(example).nodes } + it { is_expected.to eq 2 } + end + + describe '#exists?' do + subject { |example| path(example).exists? } + it { is_expected.to be true } + end + + describe '#empty?' do + subject { |example| path(example).empty? } + it { is_expected.to be false } end end end @@ -106,20 +123,37 @@ describe Gitlab::Ci::Build::Artifacts::Metadata::Path do subject { |example| path(example).children } it { expect(subject.count).to eq 3 } end + end - describe '#nodes', path: 'test' do - subject { |example| path(example).nodes } - it { is_expected.to eq 1 } + describe 'path/dir_1/subdir/subfile', path: 'path/dir_1/subdir/subfile' do + describe '#nodes' do + subject { |example| path(example).nodes } + it { is_expected.to eq 4 } + end end - describe '#nodes', path: 'test/' do - subject { |example| path(example).nodes } - it { is_expected.to eq 1 } + describe 'non-existent/', path: 'non-existent/' do + describe '#empty?' do + subject { |example| path(example).empty? } + it { is_expected.to be true } + end + + describe '#exists?' do + subject { |example| path(example).exists? } + it { is_expected.to be false } + end + end + + describe 'another_directory/', path: 'another_directory/' do + describe '#empty?' do + subject { |example| path(example).empty? } + it { is_expected.to be true } + end end describe '#metadata' do - let(:universe) do + let(:entries) do ['path/', 'path/file1', 'path/file2'] end @@ -128,21 +162,9 @@ describe Gitlab::Ci::Build::Artifacts::Metadata::Path do end subject do - described_class.new('path/file1', universe, metadata).metadata[:name] + described_class.new('path/file1', entries, metadata).metadata[:name] end it { is_expected.to eq '/path/file1' } end - - describe '#exists?', path: 'another_file' do - subject { |example| path(example).exists? } - it { is_expected.to be true } - end - - describe '#exists?', path: './non_existent/' do - let(:universe) { ['./something'] } - subject { |example| path(example).exists? } - - it { is_expected.to be false } - end end diff --git a/spec/lib/gitlab/ci/build/artifacts/metadata_spec.rb b/spec/lib/gitlab/ci/build/artifacts/metadata_spec.rb index 5c1a94974e8..42fbe40c890 100644 --- a/spec/lib/gitlab/ci/build/artifacts/metadata_spec.rb +++ b/spec/lib/gitlab/ci/build/artifacts/metadata_spec.rb @@ -51,9 +51,9 @@ describe Gitlab::Ci::Build::Artifacts::Metadata do end end - describe '#to_path' do - subject { metadata('').to_path } - it { is_expected.to be_an_instance_of(Gitlab::Ci::Build::Artifacts::Metadata::Path) } + describe '#to_entry' do + subject { metadata('').to_entry } + it { is_expected.to be_an_instance_of(Gitlab::Ci::Build::Artifacts::Metadata::Entry) } end describe '#full_version' do