Skip to content

Commit

Permalink
Ignore non-empty directories in cleanup file
Browse files Browse the repository at this point in the history
- Before ruby 2.5, FileUtils.rmdir silently fails when the target
directory is not empty, so the non-empty directories in cleanup file
are not removed. But the implementation of rmdir is changed in
ruby 2.5 so that it doesn’t rescue Errno::ENOTEMPTY any more.
  • Loading branch information
spilist committed Jan 3, 2019
1 parent dc22129 commit 9f0de68
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 6 deletions.
8 changes: 4 additions & 4 deletions lib/instance_agent/plugins/codedeploy/install_instruction.rb
Original file line number Diff line number Diff line change
Expand Up @@ -209,10 +209,10 @@ def execute
FileUtils.rm(@file_path)
elsif File.exist?(@file_path)
if File.directory?(@file_path)
# TODO (AWSGLUE-713): handle the exception if the directory is non-empty;
# this might mean the customer has put files in this directory and we should
# probably ignore the error and move on
FileUtils.rmdir(@file_path)
begin
FileUtils.rmdir(@file_path)
rescue Errno::ENOTEMPTY
end
else
FileUtils.rm(@file_path)
end
Expand Down
24 changes: 22 additions & 2 deletions test/instance_agent/plugins/codedeploy/install_instruction_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -249,14 +249,34 @@ class InstallInstructionTest < InstanceAgentTestCase
File.stubs(:exist?).with("test_delete_path").returns(true)
end

should "produce a command that deletes test_delete_path" do
should "use rm for a regular file" do
commands = InstallInstruction.parse_remove_commands(@parse_string)
FileUtils.expects(:rm).with("test_delete_path")
assert_not_equal nil, commands
commands.each do |command|
command.execute
end
end

should "use rmdir for a directory" do
File.stubs(:directory?).with("test_delete_path").returns(true)
commands = InstallInstruction.parse_remove_commands(@parse_string)
FileUtils.expects(:rmdir).with("test_delete_path")
commands.each do |command|
command.execute
end
end

should "ignore a non-empty directory by rescuing Errno::ENOTEMPTY" do
File.stubs(:directory?).with("test_delete_path").returns(true)
commands = InstallInstruction.parse_remove_commands(@parse_string)
FileUtils.stubs(:rmdir).raises(Errno::ENOTEMPTY)

assert_nothing_raised do
commands.each do |command|
command.execute
end
end
end
end

context "multiple files to delete" do
Expand Down

1 comment on commit 9f0de68

@pixeltrix
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason the removal of a non-empty directory is not forced? Otherwise the failure mode for this is pretty catastrophic - one day your deployments will just fail with an out of disk space error. You should have alerts on your disk space I'll grant, but seems like it could be a cause of some of the reports about CodeDeploy not cleaning up previous releases.

Please sign in to comment.