Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add backup attribute to file resource (close #124) #191

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion itamae.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ Gem::Specification.new do |spec|
spec.require_paths = ["lib"]

spec.add_runtime_dependency "thor"
spec.add_runtime_dependency "specinfra", [">= 2.37.0", "< 3.0.0"]
spec.add_runtime_dependency "specinfra", [">= 2.54.0", "< 3.0.0"]
spec.add_runtime_dependency "hashie"
spec.add_runtime_dependency "ansi"
spec.add_runtime_dependency "schash", "~> 0.1.0"
Expand Down
36 changes: 36 additions & 0 deletions lib/itamae/resource/file.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,15 @@
module Itamae
module Resource
class File < Base
BACKUP_PATH = '/var/itamae/backup'.freeze

define_attribute :action, default: :create
define_attribute :path, type: String, default_name: true
define_attribute :content, type: String, default: nil
define_attribute :mode, type: String
define_attribute :owner, type: String
define_attribute :group, type: String
define_attribute :backup, type: [FalseClass, Integer], default: 5
define_attribute :block, type: Proc, default: proc {}

def pre_action
Expand Down Expand Up @@ -58,6 +61,8 @@ def show_differences
end

def action_create(options)
backup if current.exist

if !current.exist && !@temppath
run_command(["touch", attributes.path])
end
Expand All @@ -84,6 +89,8 @@ def action_delete(options)
end

def action_edit(options)
backup if current.exist

if attributes.mode
run_specinfra(:change_file_mode, @temppath, attributes.mode)
else
Expand Down Expand Up @@ -183,6 +190,35 @@ def send_tempfile
f.unlink if f
end
end

def backup
return if !attributes.backup || attributes.backup <= 0

savetime = Time.now.strftime('%Y%m%d%H%M%S')
backup_filename = "#{attributes.path}.itamae-#{savetime}"
backup_path = ::File.join(BACKUP_PATH, backup_filename)
backup_directory = ::File.dirname(backup_path)

run_specinfra(:create_file_as_directory, backup_directory)
Copy link
Member

Choose a reason for hiding this comment

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

I have a concern about permission. For instance, the following situation can confuse users:

  1. User A runs recipe A
    1. Recipe A creates a backup file of /etc/foo.conf
    2. /var/itamae/backup/etc is mode 755 and owned by user A
  2. User B runs recipe B
    1. Recipe A creates a backup file of /etc/bar.conf
    2. User B can't create a directory or a file in /var/itamae/backup/etc because it is owned by user A

WDYT?

I think it is better all directories under /var/lib/itamae are 755

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryotarai Yes, I agree.

I think it is better all directories under /var/lib/itamae are 755

Isn't it mistake with 755 or 777 ?
I try to use run_command(['install', '-d', '-m=777', backup_directory]) .

run_specinfra(:change_file_mode, backup_directory, '0777')
run_specinfra(:copy_file, attributes.path, backup_path)
Copy link
Member

Choose a reason for hiding this comment

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

Files backed up should have the same permission. cp -p preserves the permission of source file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

run_specinfra(:copy_file) doesn't seem to preserve the permission.

So, I try to use run_command(['cp', '-p', attributes.path, backup_path]) .

Itamae.logger.info "#{attributes.path} backed up to #{backup_path}"

basename = ::File.basename(attributes.path)
backup_files = run_command(['ls', '-1', backup_directory])
backup_files = backup_files.stdout.chomp.split("\n").select { |f|
f.match(/\A#{basename}.itamae-[0-9]+\z/)
}.reverse
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I use run_command("ls -1 #{::File.join(BACKUP_PATH, attributes.path + '.itamae-*')}") ?
(When I use ['ls', '-1', ::File.join(BACKUP_PATH, attributes.path + '.itamae-*'] , * is escaped to \* and can't execute.)

Copy link
Member

Choose a reason for hiding this comment

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

Current implementation is good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, thank you!


if backup_files.length > attributes.backup
remainder = backup_files.slice(attributes.backup..-1)
remainder.each do |backup_to_delete|
backup_to_delete = ::File.join(backup_directory, backup_to_delete)
run_specinfra(:remove_file, backup_to_delete)
Itamae.logger.info "#{attributes.path} removed backup at #{backup_to_delete}"
end
end
end
end
end
end
12 changes: 11 additions & 1 deletion spec/integration/default_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,19 @@
end
end

describe file('/var/itamae/backup/tmp') do
it { should be_directory }
it { should be_mode 777 }
end

describe command('cat /var/itamae/backup/tmp/file.itamae-*') do
its(:stdout) { should match(/Hello World/) }
its(:exit_status) { should eq(0) }
end

Copy link
Member

Choose a reason for hiding this comment

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

Would you add tests to check backed-up file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't specify the backed-up file because file name is decided from date.
So, I try to use commmand resource with wildcard like below.
( @ryotarai Or do you have any good ideas?)

describe command('cat /var/itamae/backup/tmp/file.itamae-*') do
  its(:stdout) { should match(/Hello World/) }
  its(:exit_status) { should eq(0) }
end

Copy link
Member

Choose a reason for hiding this comment

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

So, I try to use commmand resource with wildcard like below.

LGTM

describe file('/tmp/file') do
it { should be_file }
its(:content) { should match(/Hello World/) }
its(:content) { should match(/Hello New World/) }
it { should be_mode 777 }
end

Expand Down
5 changes: 5 additions & 0 deletions spec/integration/recipes/default.rb
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,11 @@
mode "777"
end

file "/tmp/file" do
content "Hello New World"
mode "777"
end

execute "echo 'Hello Execute' > /tmp/execute"

file "/tmp/never_exist1" do
Expand Down