diff --git a/lib/pdk/module/convert.rb b/lib/pdk/module/convert.rb index ee56360f6..388fc1456 100644 --- a/lib/pdk/module/convert.rb +++ b/lib/pdk/module/convert.rb @@ -148,7 +148,7 @@ def stage_changes!(context = PDK.context) module_name = new_metadata.nil? ? 'new-module' : new_metadata.data['name'] metadata_for_render = new_metadata.nil? ? {} : new_metadata.data - template_dir.render_new_module(module_name, metadata_for_render) do |relative_file_path, file_content, file_status| + template_dir.render_new_module(module_name, metadata_for_render) do |relative_file_path, file_content, file_status, file_executable| absolute_file_path = File.join(module_dir, relative_file_path) case file_status when :unmanage @@ -156,12 +156,17 @@ def stage_changes!(context = PDK.context) when :delete update_manager.remove_file(absolute_file_path) when :init - update_manager.add_file(absolute_file_path, file_content) if convert? && !PDK::Util::Filesystem.exist?(absolute_file_path) + if convert? && !PDK::Util::Filesystem.exist?(absolute_file_path) + update_manager.add_file(absolute_file_path, file_content) + update_manager.make_file_executable(absolute_file_path) if file_executable + end when :manage if PDK::Util::Filesystem.exist?(absolute_file_path) update_manager.modify_file(absolute_file_path, file_content) + update_manager.make_file_executable(absolute_file_path) if file_executable && !PDK::Util::Filesystem.executable?(absolute_file_path) else update_manager.add_file(absolute_file_path, file_content) + update_manager.make_file_executable(absolute_file_path) if file_executable end end end diff --git a/lib/pdk/module/update_manager.rb b/lib/pdk/module/update_manager.rb index 521553480..f232a851a 100644 --- a/lib/pdk/module/update_manager.rb +++ b/lib/pdk/module/update_manager.rb @@ -11,6 +11,7 @@ def initialize @modified_files = Set.new @added_files = Set.new @removed_files = Set.new + @executable_files = Set.new @diff_cache = {} end @@ -37,6 +38,13 @@ def remove_file(path) @removed_files << path end + # Store a pending file execute mode change. + # + # @param path [String] The path to the file to be made executable. + def make_file_executable(path) + @executable_files << path + end + # Generate a summary of the changes that will be applied to the module. # # @raise (see #calculate_diffs) @@ -49,7 +57,8 @@ def changes { added: @added_files, removed: @removed_files.select { |f| PDK::Util::Filesystem.exist?(f) }, - modified: @diff_cache.compact + modified: @diff_cache.compact, + 'made executable': @executable_files } end @@ -60,7 +69,8 @@ def changes def changes? !changes[:added].empty? || !changes[:removed].empty? || - changes[:modified].any? { |_, value| !value.nil? } + changes[:modified].any? { |_, value| !value.nil? } || + !changes[:'made executable'].empty? end # Check if the update manager will change the specified file upon sync. @@ -72,13 +82,15 @@ def changes? def changed?(path) changes[:added].any? { |add| add[:path] == path } || changes[:removed].include?(path) || - changes[:modified].key?(path) + changes[:modified].key?(path) || + changes[:'made executable'].include?(path) end def clear! @modified_files.clear @added_files.clear @removed_files.clear + @executable_files.clear nil end @@ -100,6 +112,10 @@ def sync_changes! files_to_write.each do |file| write_file(file[:path], file[:content]) end + + @executable_files.each do |file| + update_execute_bits(file) + end end # Remove a file from disk. @@ -215,6 +231,16 @@ def unified_diff(path, old_content, new_content, lines_of_context = 3) output.join($INPUT_RECORD_SEPARATOR) end + + # Set the execute bits on a file + def update_execute_bits(path) + require 'pdk/util/filesystem' + + PDK.logger.debug(format("making '%{path}' executable", path: path)) + PDK::Util::Filesystem.make_executable(path) + rescue Errno::EACCES + raise PDK::CLI::ExitWithError, format("You do not have permission to make '%{path}' executable", path: path) + end end end end diff --git a/lib/pdk/template/renderer/v1/legacy_template_dir.rb b/lib/pdk/template/renderer/v1/legacy_template_dir.rb index 12192ec01..e581126c2 100644 --- a/lib/pdk/template/renderer/v1/legacy_template_dir.rb +++ b/lib/pdk/template/renderer/v1/legacy_template_dir.rb @@ -56,7 +56,7 @@ def config_for(dest_path, sync_config_path = nil) @config = conf_defaults @config.deep_merge!(@sync_config, knockout_prefix: '---') unless @sync_config.nil? end - file_config = @config.fetch(:global, {}) + file_config = @config.fetch('common', {}).clone file_config['module_metadata'] = @module_metadata file_config.merge!(@config.fetch(dest_path, {})) unless dest_path.nil? file_config.merge!(@config).tap do |c| diff --git a/lib/pdk/template/renderer/v1/renderer.rb b/lib/pdk/template/renderer/v1/renderer.rb index a410b84ab..8351cb81a 100644 --- a/lib/pdk/template/renderer/v1/renderer.rb +++ b/lib/pdk/template/renderer/v1/renderer.rb @@ -95,7 +95,9 @@ def render_module(options = {}) end end - yield dest_path, dest_content, dest_status + dest_executable = config['manage_execute_permissions'] && PDK::Util::Filesystem.executable?(File.join(template_loc, template_file)) + + yield dest_path, dest_content, dest_status, dest_executable end end # :nocov: diff --git a/lib/pdk/util/filesystem.rb b/lib/pdk/util/filesystem.rb index 83b8d5771..55930de57 100644 --- a/lib/pdk/util/filesystem.rb +++ b/lib/pdk/util/filesystem.rb @@ -27,6 +27,11 @@ def read_file(file, nil_on_error: false, open_args: 'r') end module_function :read_file + def make_executable(file) + FileUtils.chmod('a+x', file) + end + module_function :make_executable + # :nocov: # These methods just wrap core Ruby functionality and # can be ignored for code coverage @@ -133,6 +138,11 @@ def mv(*args, **kwargs) end end module_function :mv + + def executable?(*args) + File.executable?(*args) + end + module_function :executable? # :nocov: end end diff --git a/spec/unit/pdk/module/convert_spec.rb b/spec/unit/pdk/module/convert_spec.rb index 98b31e811..decfca376 100644 --- a/spec/unit/pdk/module/convert_spec.rb +++ b/spec/unit/pdk/module/convert_spec.rb @@ -58,6 +58,12 @@ def module_path(relative_path) end end + shared_context 'files made executable in the summary' do + before do + allow($stdout).to receive(:puts).with(/-Files to be made executable-/i) + end + end + shared_context 'outputs a convert report' do before do allow($stdout).to receive(:puts).with(/You can find detailed differences in convert_report.txt./) @@ -88,19 +94,20 @@ def module_path(relative_path) let(:update_manager) { instance_double(PDK::Module::UpdateManager, sync_changes!: true) } let(:template_dir) { instance_double(PDK::Template::TemplateDir, metadata: {}) } let(:metadata) { instance_double(PDK::Module::Metadata, data: {}) } - let(:template_files) { { path: 'a/path/to/file', content: 'file contents', status: :manage } } + let(:template_files) { { path: 'a/path/to/file', content: 'file contents', status: :manage, executable: true } } let(:added_files) { Set.new } let(:removed_files) { Set.new } let(:modified_files) { {} } + let(:executable_files) { Set.new } before do - changes = { added: added_files, removed: removed_files, modified: modified_files } + changes = { added: added_files, removed: removed_files, modified: modified_files, 'made executable': executable_files } allow(PDK::Module::UpdateManager).to receive(:new).and_return(update_manager) allow(instance).to receive(:update_metadata).with(any_args).and_return(metadata) allow(PDK::Template).to receive(:with).with(anything, anything).and_yield(template_dir) allow(PDK::Util::Git).to receive(:repo?).with(anything).and_return(true) - allow(template_dir).to receive(:render_new_module).and_yield(template_files[:path], template_files[:content], template_files[:status]) + allow(template_dir).to receive(:render_new_module).and_yield(template_files[:path], template_files[:content], template_files[:status], template_files[:executable]) allow(update_manager).to receive(:changes).and_return(changes) allow(update_manager).to receive(:changed?).with('Gemfile').and_return(false) allow(update_manager).to receive(:unlink_file).with('Gemfile.lock') @@ -172,6 +179,7 @@ def module_path(relative_path) context 'when the Gemfile has been modified' do include_context 'has changes in the summary' include_context 'modified files in the summary' + include_context 'files made executable in the summary' include_context 'outputs a convert report' include_context 'prompt to continue', true include_context 'completes a convert' @@ -184,6 +192,7 @@ def module_path(relative_path) allow(update_manager).to receive(:add_file).with(module_path('metadata.json'), anything) allow(update_manager).to receive(:modify_file).with(module_path(template_files[:path]), template_files[:content]) + allow(update_manager).to receive(:make_file_executable).with(module_path(template_files[:path])) allow($stdout).to receive(:puts).with(/1 files modified/) allow(update_manager).to receive(:changed?).with('Gemfile').and_return(true) allow(update_manager).to receive(:remove_file).with(anything) @@ -213,6 +222,7 @@ def module_path(relative_path) context 'when there are changes to apply' do include_context 'has changes in the summary' include_context 'modified files in the summary' + include_context 'files made executable in the summary' include_context 'outputs a convert report' include_context 'completes a convert' @@ -221,9 +231,11 @@ def module_path(relative_path) 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']) + allow($stdout).to receive(:puts).with(['some/executable/file']) allow(update_manager).to receive(:add_file).with(module_path('metadata.json'), anything) allow(update_manager).to receive(:modify_file).with(module_path(template_files[:path]), template_files[:content]) + allow(update_manager).to receive(:make_file_executable).with(module_path(template_files[:path])) allow($stdout).to receive(:puts).with(/1 files modified/) allow($stdout).to receive(:puts).with(/You can find a report of differences in convert_report.txt./) end @@ -234,11 +246,20 @@ def module_path(relative_path) } end + let(:executable_files) do + Set.new( + [ + 'some/executable/file' + ] + ) + end + context 'and run normally' do include_context 'prompt to continue', false it 'prints a diff of the changed files' do expect($stdout).to receive(:puts).with(['some/file']) + expect($stdout).to receive(:puts).with(['some/executable/file']) end it 'prompts the user to continue' do @@ -252,6 +273,36 @@ def module_path(relative_path) expect(update_manager).to receive(:sync_changes!) end + it 'lists the count of modified files' do + expect($stdout).to receive(:puts).with(/1 files modified/) + end + + it 'lists the count of files made executable' do + expect($stdout).to receive(:puts).with(/1 files made executable/) + end + + context 'when managing file execute bits' do + context 'when file exists and has correct execute bits' do + before do + allow(PDK::Util::Filesystem).to receive(:executable?).with(module_path('/a/path/to/file')).and_return(true) + end + + it 'does not stage the file for making executable' do + expect(update_manager).not_to receive(:make_file_executable).with(module_path('/a/path/to/file')) + end + end + + context 'when file exists but has incorrect execute bits' do + before do + allow(PDK::Util::Filesystem).to receive(:executable?).with(module_path('/a/path/to/file')).and_return(false) + end + + it 'stages the file for making executable' do + expect(update_manager).to receive(:make_file_executable).with(module_path('/a/path/to/file')) + end + end + end + context 'and it is to add tests' do let(:options) { { 'add-tests': true } } @@ -352,7 +403,7 @@ def module_path(relative_path) 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 } + { path: 'a/path/to/file', content: 'file contents', status: :init, executable: true } end context 'and the files already exist' do @@ -361,11 +412,16 @@ def module_path(relative_path) before do allow(PDK::Util::Filesystem).to receive(:exist?).with(module_path(template_files[:path])).and_return(true) allow(update_manager).to receive(:changes?).and_return(false) + allow(update_manager).to receive(:add_file).with(module_path('metadata.json'), anything) end it 'does not stage the file for addition' do expect(update_manager).not_to receive(:add_file).with(module_path(template_files[:path]), anything) end + + it 'does not stage the file for making executable' do + expect(update_manager).not_to receive(:make_file_executable).with(module_path(template_files[:path])) + end end context 'and the files do not exist' do @@ -373,17 +429,23 @@ def module_path(relative_path) allow(PDK::Util::Filesystem).to receive(:exist?).with(module_path(template_files[:path])).and_return(false) allow(update_manager).to receive(:changes?).and_return(true) allow(update_manager).to receive(:add_file) + allow(update_manager).to receive(:make_file_executable) end it 'stages the file for addition' do expect(update_manager).to receive(:add_file).with(module_path(template_files[:path]), template_files[:content]) end + + it 'stages the file for making executable' do + expect(update_manager).to receive(:make_file_executable).with(module_path(template_files[:path])) + 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' + include_context 'files made executable in the summary' include_context 'outputs a convert report' include_context 'completes a convert' @@ -396,12 +458,22 @@ def module_path(relative_path) ) end + let(:executable_files) do + Set.new( + [ + 'path/to/executable/file' + ] + ) + end + before do allow(update_manager).to receive(:changes?).and_return(true) allow($stdout).to receive(:puts).with(['path/to/file']) + allow($stdout).to receive(:puts).with(['path/to/executable/file']) allow(update_manager).to receive(:add_file).with(module_path('metadata.json'), anything) allow(update_manager).to receive(:add_file).with(module_path(template_files[:path]), template_files[:content]) + allow(update_manager).to receive(:make_file_executable).with(module_path(template_files[:path])) allow($stdout).to receive(:puts).with(/1 files added/) allow($stdout).to receive(:puts).with(/You can find a report of differences in convert_report.txt./) end @@ -413,6 +485,10 @@ def module_path(relative_path) expect($stdout).to receive(:puts).with(['path/to/file']) end + it 'prints a path of the files made executable' do + expect($stdout).to receive(:puts).with(['path/to/executable/file']) + end + it 'prompts the user to continue' do expect(PDK::CLI::Util).to receive(:prompt_for_yes).with(anything).and_return(false) end @@ -423,6 +499,14 @@ def module_path(relative_path) it 'syncs the pending changes' do expect(update_manager).to receive(:sync_changes!) end + + it 'lists the count of added files' do + expect($stdout).to receive(:puts).with(/1 files added/) + end + + it 'lists the count of files made executable' do + expect($stdout).to receive(:puts).with(/1 files made executable/) + end end context 'if the user chooses not to continue' do diff --git a/spec/unit/pdk/module/update_manager_spec.rb b/spec/unit/pdk/module/update_manager_spec.rb index 5a107f748..5e5b597e7 100644 --- a/spec/unit/pdk/module/update_manager_spec.rb +++ b/spec/unit/pdk/module/update_manager_spec.rb @@ -244,6 +244,44 @@ end end + describe '#make_file_executable' do + before do + update_manager.make_file_executable(dummy_file) + end + + it 'creates a pending change' do + expect(update_manager).to be_changes + end + + it 'creates a file made executable change' do + expect(update_manager.changes).to include('made executable': [dummy_file]) + end + + it 'knows that the file will be changed' do + expect(update_manager).to be_changed(dummy_file) + end + + context 'when syncing the changes' do + it 'makes the file executable' do + expect(PDK::Util::Filesystem).to receive(:make_executable).with(dummy_file) + + update_manager.sync_changes! + end + + context 'but if the file can not be written to' do + before do + allow(PDK::Util::Filesystem).to receive(:make_executable).with(dummy_file).and_raise(Errno::EACCES) + end + + it 'exits with an error' do + expect do + update_manager.sync_changes! + end.to raise_error(PDK::CLI::ExitWithError, /You do not have permission to make '#{Regexp.escape(dummy_file)}' executable/) + end + end + end + end + describe '#clear!' do before do update_manager.add_file(dummy_file, 'content') diff --git a/spec/unit/pdk/util/filesystem_spec.rb b/spec/unit/pdk/util/filesystem_spec.rb index e72635c17..8a519ba37 100644 --- a/spec/unit/pdk/util/filesystem_spec.rb +++ b/spec/unit/pdk/util/filesystem_spec.rb @@ -92,4 +92,30 @@ end end end + + describe '.make_executable' do + subject(:make_executable) { described_class.make_executable(path) } + + let(:path) { File.join('path', 'to', 'my', 'file') } + + context 'when file is writable' do + before do + allow(FileUtils).to receive(:chmod) + end + + it 'does not raise an error' do + expect { make_executable }.not_to raise_error + end + end + + context 'when file is not writable' do + before do + allow(FileUtils).to receive(:chmod).and_raise(Errno::EACCES, 'some error') + end + + it 'raises an error' do + expect { make_executable }.to raise_error(Errno::EACCES, /some error/) + end + end + end end