Skip to content

Commit

Permalink
Remove empty Carrierwave directories when asset file is removed
Browse files Browse the repository at this point in the history
Since #373 we've been deleting an asset's underlying file from the
filesystem once the file has been uploaded to S3. However, this was
leaving behind a bunch of empty directories. This change means that the
app now removes all empty parent directories when the file is removed.

The immediate motivation behind this change is that once
alphagov/govuk-puppet#7016 is applied in production, the Asset Manager
Carrierwave directory will no longer be sync-ed from the Asset Master to
the Asset Slaves and there will therefore be a danger that the
asset_master_and_slave_disk_space_similar_from_xxx Icinga alert [1]
might eventually be triggered by the non-zero disk space taken up by the
empty directories.

The implementation makes use of `FileUtils.rmdir` with the `parents`
option which is equivalent to the `rmdir -p` unix command and which
recursively removes empty directories from the supplied path upwards.

Note that I think this means that in some circumstances top-level
directories (e.g. `tmp/test_uploads/assets` in the test environment)
might be removed, but I'm pretty confident Carrierwave creates
directories using the equivalent of `mkdir -p` and so any missing
intermediate directories will be created.

[]1:
https://github.com/alphagov/govuk-puppet/blob/19837b4b351d97d84570bd8a7d01ef3420fbef94/modules/govuk/manifests/node/s_asset_slave.pp#L50-L63
  • Loading branch information
floehopper committed Jan 9, 2018
1 parent 0da9780 commit 6153fc1
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 0 deletions.
2 changes: 2 additions & 0 deletions app/models/asset.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@ class Asset
end

after_transition to: :uploaded do |asset, _|
path = asset.file.path
asset.remove_file!
FileUtils.rmdir(File.dirname(path), parents: true)
end
end

Expand Down
70 changes: 70 additions & 0 deletions spec/models/asset_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,10 @@
context 'when asset is clean' do
let(:asset) { FactoryBot.create(:clean_asset) }
let(:path) { asset.file.path }
let(:pathname) { Pathname.new(path) }
let(:dir) { pathname.parent }
let(:parent_dir) { pathname.parent.parent }
let(:grandparent_dir) { pathname.parent.parent.parent }

it 'changes asset state to uploaded' do
asset.upload_success!
Expand All @@ -543,6 +547,72 @@

expect(File.exist?(path)).to be_falsey
end

it 'removes the empty directory' do
asset.upload_success!

expect(Dir).not_to exist(dir), "#{dir} exists"
end

context 'when directory is not empty' do
before do
FileUtils.touch(dir.join('do-not-delete-me'))
end

after do
FileUtils.rm(dir.join('do-not-delete-me'), force: true)
end

it 'does not remove directory' do
asset.upload_success!

expect(Dir).to exist(dir), "#{dir} exists"
end
end

it 'removes the empty parent directory' do
asset.upload_success!

expect(Dir).not_to exist(parent_dir), "#{parent_dir} exists"
end

context 'when parent directory is not empty' do
before do
FileUtils.touch(parent_dir.join('do-not-delete-me'))
end

after do
FileUtils.rm(parent_dir.join('do-not-delete-me'), force: true)
end

it 'does not remove the parent directory' do
asset.upload_success!

expect(Dir).to exist(parent_dir), "#{parent_dir} exists"
end
end

it 'removes the empty grandparent directory' do
asset.upload_success!

expect(Dir).not_to exist(grandparent_dir), "#{grandparent_dir} exists"
end

context 'when grandparent directory is not empty' do
before do
FileUtils.touch(grandparent_dir.join('do-not-delete-me'))
end

after do
FileUtils.rm(grandparent_dir.join('do-not-delete-me'), force: true)
end

it 'does not remove the grandparent directory if not empty' do
asset.upload_success!

expect(Dir).to exist(grandparent_dir), "#{grandparent_dir} exists"
end
end
end

context 'when asset is infected' do
Expand Down

0 comments on commit 6153fc1

Please sign in to comment.