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

Fixes #16740 - Access host params through macro #3983

Merged
merged 1 commit into from Jan 3, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/models/concerns/host_common.rb
Expand Up @@ -146,10 +146,12 @@ def crypt_root_pass
end

def param_true?(name)
Foreman::Deprecation.renderer_deprecation('1.17', __method__, 'host_param_true?')
params.has_key?(name) && Foreman::Cast.to_bool(params[name])
end

def param_false?(name)
Foreman::Deprecation.renderer_deprecation('1.17', __method__, 'host_param_false?')
params.has_key?(name) && Foreman::Cast.to_bool(params[name]) == false
end

Expand Down
1 change: 1 addition & 0 deletions app/models/concerns/host_params.rb
Expand Up @@ -9,6 +9,7 @@ module HostParams
attr_reader :cached_host_params

def params
Foreman::Deprecation.renderer_deprecation('1.17', __method__, 'host_param') unless caller.first.match(/renderer\.rb.*host_param/)
host_params.update(lookup_keys_params)
end

Expand Down
24 changes: 15 additions & 9 deletions app/models/host/managed.rb
Expand Up @@ -383,6 +383,10 @@ def puppetclasses_names
# rubocop:disable Metrics/PerceivedComplexity
# rubocop:disable Metrics/CyclomaticComplexity
def info
renderer_regex = /renderer\.rb.*host_enc/
unless caller.first.match(renderer_regex) || caller[1].match(renderer_regex)
Foreman::Deprecation.renderer_deprecation('1.17', __method__, 'host_enc')
end
# Static parameters
param = {}
# maybe these should be moved to the common parameters, leaving them in for now
Expand Down Expand Up @@ -422,16 +426,8 @@ def info
# Parse ERB values contained in the parameters
param = SafeRender.new(:variables => { :host => self }).parse(param)

classes = if self.environment.nil?
[]
elsif Setting[:Parametrized_Classes_in_ENC] && Setting[:Enable_Smart_Variables_in_ENC]
lookup_keys_class_params
else
self.puppetclasses_names
end

info_hash = {}
info_hash['classes'] = classes
info_hash['classes'] = self.enc_puppetclasses
info_hash['parameters'] = param
info_hash['environment'] = param["foreman_env"] if Setting["enc_environment"] && param["foreman_env"]

Expand Down Expand Up @@ -930,6 +926,16 @@ def firmware_type
Operatingsystem.firmware_type(pxe_loader)
end

def enc_puppetclasses
if self.environment.nil?
[]
elsif Setting[:Parametrized_Classes_in_ENC] && Setting[:Enable_Smart_Variables_in_ENC]
lookup_keys_class_params
else
self.puppetclasses_names
end
end

private

def compute_profile_present?
Expand Down
18 changes: 17 additions & 1 deletion app/services/foreman/deprecation.rb
Expand Up @@ -2,12 +2,28 @@ module Foreman
class Deprecation
#deadline_version - is the version the deprecation is going to be deleted, the format must be a major release e.g "1.8"
def self.deprecation_warning(foreman_version_deadline, info)
raise Foreman::Exception.new(N_("Invalid version format, please enter in x.y (only major version).")) unless foreman_version_deadline.to_s.match(/\A\d[.]\d+\z/)
check_version_format foreman_version_deadline
ActiveSupport::Deprecation.warn("You are using a deprecated behavior, it will be removed in version #{foreman_version_deadline}, #{info}", caller(2))
end

def self.check_version_format(foreman_version_deadline)
raise Foreman::Exception.new(N_("Invalid version format, please enter in x.y (only major version).")) unless foreman_version_deadline.to_s.match(/\A\d[.]\d+\z/)
end

def self.api_deprecation_warning(info)
ActiveSupport::Deprecation.warn("Your API call uses deprecated behavior, #{info}", caller)
end

def self.renderer_deprecation(foreman_version_deadline, method, new_method)
check_version_format foreman_version_deadline
called_from_params = false
caller.each_with_index do |item, index|
called_from_params = true if item.match(/host_params\.rb.*params/)
return if called_from_params && item.match(/managed\.rb.*info/)
next unless item.match(/renderer\.rb.*render_safe/)
Copy link
Contributor

@domcleal domcleal Dec 14, 2016

Choose a reason for hiding this comment

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

When using host_param in a template, two deprecation warnings seem to be logged. Please check this works properly. A simple unit test that fails:

test 'should render host param using "host_param" helper without deprecation' do
  @renderer.host = FactoryGirl.create(:host, :with_puppet)
  Rails.logger.expects(:warn).never
  assert @renderer.render_safe("<%= host_param('test') %>", DummyRenderer::ALLOWED_HELPERS).present?
end

The logic on this line seems incorrect to me, it skips the deprecation warning only from render_safe. Surely any other entry in the callstack would trigger this?

I'm unsure why template rendering triggers two deprecation warnings without further investigation, perhaps something else internally is calling @host.params and triggering a second. Please check it's working correctly in real usage.

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 fixed the problem with @host.params being called internally, similar thing was happening to host_enc as well. I added tests to cover these cases.

Rails.logger.warn "DEPRECATION WARNING: you are using deprecated @host.#{method} in a template, it will be removed in #{foreman_version_deadline}. Use #{new_method} instead."
return
end
end
end
end
28 changes: 27 additions & 1 deletion lib/foreman/renderer.rb
Expand Up @@ -14,7 +14,9 @@ def default_url_options
:foreman_server_url, :log_debug, :log_info, :log_warn, :log_error, :log_fatal, :template_name, :dns_lookup,
:pxe_kernel_options ]
ALLOWED_HOST_HELPERS ||= [ :grub_pass, :ks_console, :root_pass,
:media_path, :param_true?, :param_false?, :match ]
:media_path, :param_true?, :param_false?, :match,
:host_param_true?, :host_param_false?,
:host_param, :host_puppet_classes, :host_enc ]

ALLOWED_HELPERS ||= ALLOWED_GENERIC_HELPERS + ALLOWED_HOST_HELPERS

Expand All @@ -26,6 +28,30 @@ def template_logger
@template_logger ||= Foreman::Logging.logger('templates')
end

def host_enc(*path)
@enc ||= @host.info.deep_dup
return @enc if path.compact.empty?
enc = @enc
path.each { |step| enc = enc.fetch step }
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing test coverage of this branch, it only covers path.compact.empty?.

enc
Copy link
Contributor

Choose a reason for hiding this comment

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

No duplication here, but it does above if there is no path? This should probably dup too.

end

def host_param(param_name)
@host.params[param_name]
end

def host_puppet_classes
@host.puppetclasses
end

def host_param_true?(name)
@host.params.has_key?(name) && Foreman::Cast.to_bool(@host.params[name])
end

def host_param_false?(name)
@host.params.has_key?(name) && Foreman::Cast.to_bool(@host.params[name]) == false
end

def render_safe(template, allowed_methods = [], allowed_vars = {})
if Setting[:safemode_render]
box = Safemode::Box.new self, allowed_methods
Expand Down
39 changes: 39 additions & 0 deletions test/unit/foreman/renderer_deprecation_test.rb
@@ -0,0 +1,39 @@
require 'test_helper'

class RendererTest < ActiveSupport::TestCase
class DummyRenderer
attr_accessor :host

include Foreman::Renderer
end

test 'should show the deprecation for @host.info' do
renderer = DummyRenderer.new
renderer.host = FactoryGirl.create(:host, :with_puppet)
template = FactoryGirl.create(:provisioning_template, :template => '<%= @host.info %>')
Rails.logger.expects(:warn).with("DEPRECATION WARNING: you are using deprecated @host.info in a template, it will be removed in 1.17. Use host_enc instead.").once
renderer.unattended_render template
end

test 'should show the deprecation for @host.params' do
renderer = DummyRenderer.new
renderer.host = FactoryGirl.create(:host, :with_puppet)
template = FactoryGirl.create(:provisioning_template, :template => '<%= @host.params %>')
Rails.logger.expects(:warn).with("DEPRECATION WARNING: you are using deprecated @host.params in a template, it will be removed in 1.17. Use host_param instead.").once
renderer.unattended_render template
end

test 'should render host param using "host_param" helper without deprecation' do
renderer = DummyRenderer.new
renderer.host = FactoryGirl.create(:host, :with_puppet)
Rails.logger.expects(:warn).never
assert renderer.render_safe("<%= host_param('test') %>", DummyRenderer::ALLOWED_HELPERS).present?
end

test 'should render host info using "host_enc" helper without deprecation' do
renderer = DummyRenderer.new
renderer.host = FactoryGirl.create(:host, :with_puppet)
Rails.logger.expects(:warn).never
assert renderer.render_safe("<%= host_enc %>", DummyRenderer::ALLOWED_HELPERS).present?
end
end
33 changes: 33 additions & 0 deletions test/unit/foreman/renderer_test.rb
Expand Up @@ -221,4 +221,37 @@ def setup_safemode_renderer
@renderer.instance_variable_set '@whatever_random_name', 'has_value'
assert_equal({ :whatever_random_name => 'has_value' }, @renderer.send(:allowed_variables_mapping, [ :whatever_random_name ]))
end

test 'should render puppetclasses using host_puppetclasses helper' do
@renderer.host = FactoryGirl.create(:host, :with_puppetclass)
assert @renderer.host_puppet_classes
end

test 'should render host param using "host_param" helper' do
@renderer.host = FactoryGirl.create(:host, :with_puppet)
assert @renderer.host_param('test').present?
end

test 'should have host_param_true? helper' do
@renderer.host = FactoryGirl.create(:host, :with_puppet)
FactoryGirl.create(:parameter, :name => 'true_param', :value => "true")
assert @renderer.host_param_true?('true_param')
end

test 'should have host_param_false? helper' do
@renderer.host = FactoryGirl.create(:host, :with_puppet)
FactoryGirl.create(:parameter, :name => 'false_param', :value => "false")
assert @renderer.host_param_false?('false_param')
end

test 'should have host_enc helper' do
@renderer.host = FactoryGirl.create(:host, :with_puppet)
assert @renderer.host_enc
end

test "should find path in host_enc" do
host = FactoryGirl.create(:host, :with_puppet)
@renderer.host = host
assert_equal host.puppetmaster, @renderer.host_enc('parameters', 'puppetmaster')
end
end