Skip to content
Merged
14 changes: 9 additions & 5 deletions lib/octocatalog-diff/catalog-util/builddir.rb
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,14 @@ def create_symlinks(logger = nil)
install_directory_symlink(logger, File.join(@options[:basedir], x), x)
end
else
if @options[:environment]
logger.warn '--environment is ignored unless --preserve-environments is used' unless logger.nil?
end
if @options[:create_symlinks]
if @options[:create_symlinks] && @options[:environment]
unless logger.nil?
logger.warn '--create-symlinks with --environment ignored unless --preserve-environments is used'
end
elsif @options[:create_symlinks]
logger.warn '--create-symlinks is ignored unless --preserve-environments is used' unless logger.nil?
elsif @options[:environment]
return install_directory_symlink(logger, @options[:basedir], "environments/#{@options[:environment]}")
end
install_directory_symlink(logger, @options[:basedir])
end
Expand Down Expand Up @@ -178,6 +181,7 @@ def install_fact_file(logger, options)
def install_directory_symlink(logger, dir, target = 'environments/production')
raise ArgumentError, "Called install_directory_symlink with #{dir.class} argument" unless dir.is_a?(String)
raise Errno::ENOENT, "Specified directory #{dir} doesn't exist" unless File.directory?(dir)

symlink_target = File.join(@tempdir, target)

if target =~ %r{/}
Expand Down Expand Up @@ -330,7 +334,7 @@ def install_ssl_client_password(logger, password)
end

def environment
@options[:preserve_environments] ? @options.fetch(:environment, 'production') : 'production'
@options.fetch(:environment, 'production')

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe there should be a protection here to block environment and create-symlinks being used together, as the debug output on 89 states.

@kpaulisse kpaulisse Feb 26, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I looked into this and you're actually very close here. There is a method in https://github.com/github/octocatalog-diff/blob/master/lib/octocatalog-diff/catalog/computed.rb#L82-L84 that needs to be updated to match this change. Also some spec tests need to be updated for the new behavior.

Have a look at https://gist.github.com/kpaulisse/7bda51e395aab49585aa9933d78fe724 for what I had to do to get the tests passing. Hopefully this is a push in the right direction - I'm 👍 to the change in general.

end
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/octocatalog-diff/catalog/computed.rb
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ def compilation_dir

# Environment used to compile catalog
def environment
@opts[:preserve_environments] ? @opts.fetch(:environment, 'production') : 'production'
@options.fetch(:environment, 'production')

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oops, opts.

end

private
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,20 +40,16 @@
'-n', 'rspec-node.github.net',
'--environment', 'asdfgh',
'--to-catalog', OctocatalogDiff::Spec.fixture_path('catalogs/default-catalog-v4.json'),
'--hiera-config', 'environments/production/config/hiera.yaml',
'--hiera-config', 'config/hiera.yaml',
'--hiera-path-strip', '/var/lib/puppet', '--no-parallel'
]
)
end

it 'should exit without error' do
expect(@result.exitcode).to eq(0), OctocatalogDiff::Integration.format_exception(@result)
expect(@result.exitcode).to eq(2), OctocatalogDiff::Integration.format_exception(@result)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This one should be reverted as per my earlier comment - change the 2 back to a 0 here.

expect(@result.exception).to be_nil, OctocatalogDiff::Integration.format_exception(@result)
end

it 'should log warning about --environment being useless in this context' do
expect(@result.logs).to match(/WARN -- : --environment is ignored unless --preserve-environments is used/)
end
end

context 'with --create-symlinks set' do
Expand Down
43 changes: 41 additions & 2 deletions spec/octocatalog-diff/tests/catalog-util/builddir_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -121,15 +121,18 @@
end
end

context 'with --environment' do
context 'with --create-symlinks and --environment' do
context 'with logger' do
it 'should log a warning message and install default symlink' do
logger = double('Logger')
expect(logger).to receive(:warn).with('--environment is ignored unless --preserve-environments is used')
expect(logger).to receive(:warn).with(
'--create-symlinks with --environment ignored unless --preserve-environments is used'
)

@described_object.instance_variable_set(
'@options',
basedir: '/tmp/basedir',
create_symlinks: %w(foo bar),
environment: 'baz'
)

Expand All @@ -146,6 +149,7 @@
@described_object.instance_variable_set(
'@options',
basedir: '/tmp/basedir',
create_symlinks: %w(foo bar),
environment: 'baz'
)

Expand All @@ -157,6 +161,41 @@
end
end
end
context 'with --environment' do
context 'with logger' do
it 'should install a symlink to the given environment' do
logger = double('Logger')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The pattern elsewhere in the tests is to actually construct a real logger using:

logger, logger_str = OctocatalogDiff::Spec.setup_logger

You can then also test logger_str.string to be matching text if you want to verify log messages.

I realize that line 127 didn't use this pattern - must have been one I missed when I went through and tried to update all of them long, long ago... Would you mind doing it the new way? 😸


@described_object.instance_variable_set(
'@options',
basedir: '/tmp/basedir',
environment: 'baz'
)

expect(@described_object)
.to receive(:install_directory_symlink)
.with(logger, '/tmp/basedir', 'environments/baz')

expect { @described_object.send(:create_symlinks, logger) }.not_to raise_error
end
end

context 'without logger' do
it 'should install default symlink' do
@described_object.instance_variable_set(
'@options',
basedir: '/tmp/basedir',
environment: 'baz'
)

expect(@described_object)
.to receive(:install_directory_symlink)
.with(nil, '/tmp/basedir', 'environments/baz')

expect { @described_object.send(:create_symlinks) }.not_to raise_error
end
end
end

context 'without --create-symlinks or --environment' do
it 'should install directory symlink' do
Expand Down
10 changes: 6 additions & 4 deletions spec/octocatalog-diff/tests/catalog/computed_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -422,17 +422,19 @@
context 'when preserve_environments is not set' do
context 'and environment is specified' do
it 'should return without raising error when directory exists' do
allow(File).to receive(:"directory?").with('/tmp/assert/environments/production').and_return(true)
allow(File).to receive(:"directory?").with('/tmp/assert/environments/production').and_return(false)
allow(File).to receive(:"directory?").with('/tmp/assert/environments/custom').and_return(true)
described_object = described_class.allocate
described_object.instance_variable_set('@opts', environment: 'nope')
described_object.instance_variable_set('@opts', environment: 'custom')
described_object.instance_variable_set('@builddir', OpenStruct.new(tempdir: '/tmp/assert'))
expect { described_object.send(:assert_that_puppet_environment_directory_exists) }.not_to raise_error
end

it 'should raise Errno::ENOENT when directory does not exist' do
allow(File).to receive(:"directory?").with('/tmp/assert/environments/production').and_return(false)
allow(File).to receive(:"directory?").with('/tmp/assert/environments/production').and_return(true)
allow(File).to receive(:"directory?").with('/tmp/assert/environments/custom').and_return(false)
described_object = described_class.allocate
described_object.instance_variable_set('@opts', environment: 'yup')
described_object.instance_variable_set('@opts', environment: 'custom')
described_object.instance_variable_set('@builddir', OpenStruct.new(tempdir: '/tmp/assert'))
expect { described_object.send(:assert_that_puppet_environment_directory_exists) }.to raise_error(Errno::ENOENT)
end
Expand Down