From 5c0432b07c0c10bd6fe698b9036abf5360f4d839 Mon Sep 17 00:00:00 2001 From: Tim Sharpe Date: Fri, 2 Aug 2019 16:10:26 +1000 Subject: [PATCH 1/3] (maint) Convert PDK::Module::Metadata to use PDK::Util::Filesystem --- lib/pdk/module/metadata.rb | 6 +++--- lib/pdk/util/filesystem.rb | 5 +++++ spec/unit/pdk/module/metadata_spec.rb | 24 ++++++++++++------------ 3 files changed, 20 insertions(+), 15 deletions(-) diff --git a/lib/pdk/module/metadata.rb b/lib/pdk/module/metadata.rb index 22d5e17f0..5c170418b 100644 --- a/lib/pdk/module/metadata.rb +++ b/lib/pdk/module/metadata.rb @@ -94,16 +94,16 @@ def self.from_file(metadata_json_path) raise ArgumentError, _('Cannot read metadata from file: no path to file was given.') end - unless File.file?(metadata_json_path) + unless PDK::Util::Filesystem.file?(metadata_json_path) raise ArgumentError, _("'%{file}' does not exist or is not a file.") % { file: metadata_json_path } end - unless File.readable?(metadata_json_path) + unless PDK::Util::Filesystem.readable?(metadata_json_path) raise ArgumentError, _("Unable to open '%{file}' for reading.") % { file: metadata_json_path } end begin - data = JSON.parse(File.read(metadata_json_path)) + data = JSON.parse(PDK::Util::Filesystem.read_file(metadata_json_path)) rescue JSON::JSONError => e raise ArgumentError, _('Invalid JSON in metadata.json: %{msg}') % { msg: e.message } end diff --git a/lib/pdk/util/filesystem.rb b/lib/pdk/util/filesystem.rb index e81f1a0ec..505903b30 100644 --- a/lib/pdk/util/filesystem.rb +++ b/lib/pdk/util/filesystem.rb @@ -49,6 +49,11 @@ def fnmatch(*args) File.fnmatch(*args) end module_function :fnmatch + + def readable?(*args) + File.readable?(*args) + end + module_function :readable? #:nocov: end end diff --git a/spec/unit/pdk/module/metadata_spec.rb b/spec/unit/pdk/module/metadata_spec.rb index 3fc2250de..752d2d20a 100644 --- a/spec/unit/pdk/module/metadata_spec.rb +++ b/spec/unit/pdk/module/metadata_spec.rb @@ -15,18 +15,18 @@ end it 'can populate itself from a metadata.json file on disk' do - allow(File).to receive(:file?).with(metadata_json_path).and_return(true) - allow(File).to receive(:readable?).with(metadata_json_path).and_return(true) + allow(PDK::Util::Filesystem).to receive(:file?).with(metadata_json_path).and_return(true) + allow(PDK::Util::Filesystem).to receive(:readable?).with(metadata_json_path).and_return(true) allow(PDK::Util).to receive(:package_install?).and_return(false) - allow(File).to receive(:read).with(metadata_json_path).and_return(metadata_json_content) + allow(PDK::Util::Filesystem).to receive(:read_file).with(metadata_json_path).and_return(metadata_json_content) expect(described_class.from_file(metadata_json_path).data).to include('name' => 'foo-bar', 'version' => '0.1.0') end it 'can populate itself from a metadata.json file on disk with a trailing newline' do - allow(File).to receive(:file?).with(metadata_json_path).and_return(true) - allow(File).to receive(:readable?).with(metadata_json_path).and_return(true) - allow(File).to receive(:read).with(metadata_json_path).and_return(metadata_json_content + "\n") + allow(PDK::Util::Filesystem).to receive(:file?).with(metadata_json_path).and_return(true) + allow(PDK::Util::Filesystem).to receive(:readable?).with(metadata_json_path).and_return(true) + allow(PDK::Util::Filesystem).to receive(:read_file).with(metadata_json_path).and_return(metadata_json_content + "\n") expect(described_class.from_file(metadata_json_path).data).to include('name' => 'foo-bar', 'version' => '0.1.0') end @@ -36,21 +36,21 @@ end it 'raises an ArgumentError if the file does not exist' do - allow(File).to receive(:file?).with(metadata_json_path).and_return(false) + allow(PDK::Util::Filesystem).to receive(:file?).with(metadata_json_path).and_return(false) expect { described_class.from_file(metadata_json_path) }.to raise_error(ArgumentError, %r{'#{metadata_json_path}'.*not exist}) end it 'raises an ArgumentError if the file exists but is not readable' do - allow(File).to receive(:file?).with(metadata_json_path).and_return(true) - allow(File).to receive(:readable?).with(metadata_json_path).and_return(false) + allow(PDK::Util::Filesystem).to receive(:file?).with(metadata_json_path).and_return(true) + allow(PDK::Util::Filesystem).to receive(:readable?).with(metadata_json_path).and_return(false) expect { described_class.from_file(metadata_json_path) }.to raise_error(ArgumentError, %r{Unable to open '#{metadata_json_path}'}) end it 'raises an ArgumentError if the file contains invalid JSON' do - allow(File).to receive(:file?).with(metadata_json_path).and_return(true) - allow(File).to receive(:readable?).with(metadata_json_path).and_return(true) - allow(File).to receive(:read).with(metadata_json_path).and_return('{"foo": }') + allow(PDK::Util::Filesystem).to receive(:file?).with(metadata_json_path).and_return(true) + allow(PDK::Util::Filesystem).to receive(:readable?).with(metadata_json_path).and_return(true) + allow(PDK::Util::Filesystem).to receive(:read_file).with(metadata_json_path).and_return('{"foo": }') expect { described_class.from_file(metadata_json_path) }.to raise_error(ArgumentError, %r{Invalid JSON.*unexpected token}) end From b5b13005fb81ae89d5e238cb44b5f61d2084ed7d Mon Sep 17 00:00:00 2001 From: Tim Sharpe Date: Fri, 2 Aug 2019 16:13:51 +1000 Subject: [PATCH 2/3] (maint) Convert PDK::Module::Convert to use PDK::Util::Filesystem --- lib/pdk/module/convert.rb | 10 +++++----- lib/pdk/util/filesystem.rb | 5 +++++ spec/unit/pdk/module/convert_spec.rb | 20 ++++++++++---------- 3 files changed, 20 insertions(+), 15 deletions(-) diff --git a/lib/pdk/module/convert.rb b/lib/pdk/module/convert.rb index 65f15f579..d5f8f0ed7 100644 --- a/lib/pdk/module/convert.rb +++ b/lib/pdk/module/convert.rb @@ -74,7 +74,7 @@ def stage_changes! if options[:noop] && new_metadata.nil? update_manager.add_file(metadata_path, '') - elsif File.file?(metadata_path) + elsif PDK::Util::Filesystem.file?(metadata_path) update_manager.modify_file(metadata_path, new_metadata.to_json) else update_manager.add_file(metadata_path, new_metadata.to_json) @@ -86,7 +86,7 @@ def stage_changes! elsif file_status == :delete update_manager.remove_file(file_path) elsif file_status == :manage - if File.exist? file_path + if PDK::Util::Filesystem.exist?(file_path) update_manager.modify_file(file_path, file_content) else update_manager.add_file(file_path, file_content) @@ -107,8 +107,8 @@ def template_uri end def update_metadata(metadata_path, template_metadata) - if File.file?(metadata_path) - unless File.readable?(metadata_path) + if PDK::Util::Filesystem.file?(metadata_path) + unless PDK::Util::Filesystem.readable?(metadata_path) raise PDK::CLI::ExitWithError, _('Unable to update module metadata; %{path} exists but it is not readable.') % { path: metadata_path, } @@ -124,7 +124,7 @@ def update_metadata(metadata_path, template_metadata) rescue ArgumentError metadata = PDK::Generate::Module.prepare_metadata(options) unless options[:noop] end - elsif File.exist?(metadata_path) + elsif PDK::Util::Filesystem.exist?(metadata_path) raise PDK::CLI::ExitWithError, _('Unable to update module metadata; %{path} exists but it is not a file.') % { path: metadata_path, } diff --git a/lib/pdk/util/filesystem.rb b/lib/pdk/util/filesystem.rb index 505903b30..a7b47e360 100644 --- a/lib/pdk/util/filesystem.rb +++ b/lib/pdk/util/filesystem.rb @@ -54,6 +54,11 @@ def readable?(*args) File.readable?(*args) end module_function :readable? + + def exist?(*args) + File.exist?(*args) + end + module_function :exist? #:nocov: end end diff --git a/spec/unit/pdk/module/convert_spec.rb b/spec/unit/pdk/module/convert_spec.rb index 4ff28ccf7..053b5baca 100644 --- a/spec/unit/pdk/module/convert_spec.rb +++ b/spec/unit/pdk/module/convert_spec.rb @@ -120,7 +120,7 @@ include_context 'no changes in the summary' before(:each) do - allow(File).to receive(:exist?).with('a/path/to/file').and_return(true) + allow(PDK::Util::Filesystem).to receive(:exist?).with('a/path/to/file').and_return(true) allow(update_manager).to receive(:changes?).and_return(false) allow(template_dir).to receive(:render) allow(PDK::Module::TemplateDir).to receive(:files_in_template).and_return({}) @@ -141,7 +141,7 @@ include_context 'completes a convert' before(:each) do - allow(File).to receive(:exist?).with('a/path/to/file').and_return(true) + allow(PDK::Util::Filesystem).to receive(:exist?).with('a/path/to/file').and_return(true) allow(update_manager).to receive(:modify_file).with(any_args) allow(update_manager).to receive(:changes?).and_return(true) allow($stdout).to receive(:puts).with(['Gemfile']) @@ -182,7 +182,7 @@ include_context 'completes a convert' before(:each) do - allow(File).to receive(:exist?).with('a/path/to/file').and_return(true) + allow(PDK::Util::Filesystem).to receive(:exist?).with('a/path/to/file').and_return(true) allow(update_manager).to receive(:modify_file).with(any_args) allow(update_manager).to receive(:changes?).and_return(true) allow($stdout).to receive(:puts).with(['some/file']) @@ -274,7 +274,7 @@ end before(:each) do - allow(File).to receive(:exist?).with('a/path/to/file').and_return(false) + allow(PDK::Util::Filesystem).to receive(:exist?).with('a/path/to/file').and_return(false) allow(update_manager).to receive(:changes?).and_return(true) allow($stdout).to receive(:puts).with(['path/to/file']) @@ -396,18 +396,18 @@ context 'when the metadata file exists' do before(:each) do - allow(File).to receive(:exist?).with(metadata_path).and_return(true) + allow(PDK::Util::Filesystem).to receive(:exist?).with(metadata_path).and_return(true) end context 'and is a file' do before(:each) do - allow(File).to receive(:file?).with(metadata_path).and_return(true) + allow(PDK::Util::Filesystem).to receive(:file?).with(metadata_path).and_return(true) end context 'and is readable' do before(:each) do - allow(File).to receive(:readable?).with(metadata_path).and_return(true) - allow(File).to receive(:read).with(metadata_path).and_return(existing_metadata) + allow(PDK::Util::Filesystem).to receive(:readable?).with(metadata_path).and_return(true) + allow(PDK::Util::Filesystem).to receive(:read_file).with(metadata_path).and_return(existing_metadata) end let(:existing_metadata) do @@ -453,7 +453,7 @@ context 'and is not readable' do before(:each) do - allow(File).to receive(:readable?).with(metadata_path).and_return(false) + allow(PDK::Util::Filesystem).to receive(:readable?).with(metadata_path).and_return(false) end it 'exits with an error' do @@ -466,7 +466,7 @@ context 'and is not a file' do before(:each) do - allow(File).to receive(:file?).with(metadata_path).and_return(false) + allow(PDK::Util::Filesystem).to receive(:file?).with(metadata_path).and_return(false) end it 'exits with an error' do From 48c2abed197a81f83c33ec1d2dd1e6872014f6a4 Mon Sep 17 00:00:00 2001 From: Tim Sharpe Date: Mon, 12 Aug 2019 10:53:23 +1000 Subject: [PATCH 3/3] (PDK-1363) Apply init templates during module convert --- lib/pdk/module/convert.rb | 17 ++++++++++--- lib/pdk/module/templatedir.rb | 7 ++++- spec/acceptance/convert_spec.rb | 17 +++++++++++++ spec/acceptance/update_spec.rb | 16 ++++++++++++ spec/unit/pdk/module/convert_spec.rb | 38 ++++++++++++++++++++++++++++ spec/unit/pdk/module/update_spec.rb | 6 +++++ 6 files changed, 96 insertions(+), 5 deletions(-) diff --git a/lib/pdk/module/convert.rb b/lib/pdk/module/convert.rb index d5f8f0ed7..02de51c05 100644 --- a/lib/pdk/module/convert.rb +++ b/lib/pdk/module/convert.rb @@ -16,6 +16,10 @@ def initialize(options = {}) @options = options end + def convert? + instance_of?(PDK::Module::Convert) + end + def run stage_changes! @@ -68,7 +72,7 @@ def needs_bundle_update? def stage_changes! metadata_path = 'metadata.json' - PDK::Module::TemplateDir.new(template_uri, nil, false) do |templates| + PDK::Module::TemplateDir.new(template_uri, nil, true) do |templates| new_metadata = update_metadata(metadata_path, templates.metadata) templates.module_metadata = new_metadata.data unless new_metadata.nil? @@ -81,11 +85,16 @@ def stage_changes! end templates.render do |file_path, file_content, file_status| - if file_status == :unmanage + case file_status + when :unmanage PDK.logger.debug(_("skipping '%{path}'") % { path: file_path }) - elsif file_status == :delete + when :delete update_manager.remove_file(file_path) - elsif file_status == :manage + when :init + if convert? && !PDK::Util::Filesystem.exist?(file_path) + update_manager.add_file(file_path, file_content) + end + when :manage if PDK::Util::Filesystem.exist?(file_path) update_manager.modify_file(file_path, file_content) else diff --git a/lib/pdk/module/templatedir.rb b/lib/pdk/module/templatedir.rb index 2c1e13506..1006394a2 100644 --- a/lib/pdk/module/templatedir.rb +++ b/lib/pdk/module/templatedir.rb @@ -122,7 +122,12 @@ def render PDK.logger.debug(_("Rendering '%{template}'...") % { template: template_file }) dest_path = template_file.sub(%r{\.erb\Z}, '') config = config_for(dest_path) - dest_status = :manage + + dest_status = if template_loc.start_with?(@moduleroot_init) + :init + else + :manage + end if config['unmanaged'] dest_status = :unmanage diff --git a/spec/acceptance/convert_spec.rb b/spec/acceptance/convert_spec.rb index 6d58a8180..9ce455236 100644 --- a/spec/acceptance/convert_spec.rb +++ b/spec/acceptance/convert_spec.rb @@ -135,4 +135,21 @@ it { is_expected.not_to be_file } end end + + context 'when an init-only templated file is missing' do + include_context 'in a new module', 'init_missing', template: template_repo + + before(:all) do + FileUtils.rm_f('README.md') + end + + describe command("#{pdk_convert_base} --force --skip-interview") do + its(:exit_status) { is_expected.to eq(0) } + its(:stderr) { is_expected.to have_no_output } + its(:stdout) { is_expected.to match(%r{-+files to be added-+\nREADME\.md}mi) } + describe file('README.md') do + it { is_expected.to be_file } + end + end + end end diff --git a/spec/acceptance/update_spec.rb b/spec/acceptance/update_spec.rb index 1ae3256cb..4af4c1fcd 100644 --- a/spec/acceptance/update_spec.rb +++ b/spec/acceptance/update_spec.rb @@ -62,5 +62,21 @@ end end end + + context 'that is missing an init-only templated file' do + before(:all) do + FileUtils.rm_f('README.md') + end + + describe command('pdk update --force') do + its(:exit_status) { is_expected.to eq(0) } + its(:stderr) { is_expected.to have_no_output } + its(:stdout) { is_expected.to match(%r{no changes required}i) } + + describe file('README.md') do + it { is_expected.not_to be_file } + end + end + end end end diff --git a/spec/unit/pdk/module/convert_spec.rb b/spec/unit/pdk/module/convert_spec.rb index 053b5baca..f6e0ffc7f 100644 --- a/spec/unit/pdk/module/convert_spec.rb +++ b/spec/unit/pdk/module/convert_spec.rb @@ -258,6 +258,38 @@ end end + context 'when there are init files to add' do + let(:options) { { noop: true } } + let(:template_files) do + { path: 'a/path/to/file', content: 'file contents', status: :init } + end + + context 'and the files already exist' do + include_context 'no changes in the summary' + + before(:each) do + allow(PDK::Util::Filesystem).to receive(:exist?).with(template_files[:path]).and_return(true) + allow(update_manager).to receive(:changes?).and_return(false) + end + + it 'does not stage the file for addition' do + expect(update_manager).not_to receive(:add_file).with(template_files[:path], anything) + end + end + + context 'and the files do not exist' do + before(:each) do + allow(PDK::Util::Filesystem).to receive(:exist?).with(template_files[:path]).and_return(false) + allow(update_manager).to receive(:changes?).and_return(true) + allow(update_manager).to receive(:add_file) + end + + it 'stages the file for addition' do + expect(update_manager).to receive(:add_file).with(template_files[:path], template_files[:content]) + end + end + end + context 'when there are files to add' do include_context 'has changes in the summary' include_context 'added files in the summary' @@ -344,6 +376,12 @@ end end + describe '#convert?' do + subject { described_class.new.convert? } + + it { is_expected.to be_truthy } + end + describe '#template_uri' do subject { described_class.new(options).template_uri } diff --git a/spec/unit/pdk/module/update_spec.rb b/spec/unit/pdk/module/update_spec.rb index ef5870aad..eb2812bfa 100644 --- a/spec/unit/pdk/module/update_spec.rb +++ b/spec/unit/pdk/module/update_spec.rb @@ -344,4 +344,10 @@ end end end + + describe '#convert?' do + subject { described_class.new.convert? } + + it { is_expected.to be_falsey } + end end