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 #17680 - templates_used host helper #4115

Closed
wants to merge 1 commit into from

Conversation

dLobatog
Copy link
Member

@dLobatog dLobatog commented Dec 15, 2016

templates_used contains the names of all of the provisioning templates
kickstart used during a host build. This allows users to be able to show the
name of a provisioning template during PXEBoot, for example.

Submitted in behalf of Peter Vreman.

@theforeman-bot
Copy link
Member

There were the following issues with the commit message:

  • commit message for d81e6d0 is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@mention-bot
Copy link

@dLobatog, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagasse, @ares and @GregSutcliffe to be potential reviewers.

Copy link
Member

@ares ares left a comment

Choose a reason for hiding this comment

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

could you please also add not only the test for host method but for renderer to make sure it can be rendered in template? the best place is probably here test/unit/foreman/renderer_test.rb

@@ -828,6 +828,14 @@ def available_template_kinds(provisioning = nil)
end.compact
end

def templates_used
Copy link
Member

Choose a reason for hiding this comment

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

this whole method can be written as one-liner

Hash[available_template_kinds.map { |t| [t.template_kind_name, t.name] }]

but if you find it less readable, I don't insist

@dLobatog
Copy link
Member Author

@ares I've added a test for renderer.rb that tests the method can indeed be called on Host. The test doesn't check for the contents -that's the purpose of the unit test in Host itself- but it checks that one is allowed to run templates_used. I don't think testing for the content there would make much sense as it's writing the same test twice.

Thanks for taking a look!

@@ -221,4 +221,8 @@ 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 'templates_used is allowed to render for host' do
assert Safemode.find_jail_class(Host::Managed).allowed? :templates_used
Copy link
Member

Choose a reason for hiding this comment

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

I think this test only verifies that safemode jail can list allowed methods. I agree it's not required to test the output but I'd like to have a test that verifies it can render without errors at least. Renderer is sort of a user API so I'd like to have it covered.

you could use something like this in block near line 151

    test "#{renderer_name} should render templates used" do
      send "setup_#{renderer_name}"
      @renderer.host = FactoryGirl.build(:host)
      template = mock('template')
      template.stubs(:template).returns('<%= @host.templates_used %>')
      assert_nothing_raised do
        @renderer.unattended_render(template)
      end
    end

end
end


Choose a reason for hiding this comment

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

Extra blank line detected.

@dLobatog
Copy link
Member Author

dLobatog commented Jan 4, 2017

@ares Updated with a test to check it renders correctly and displays the names

templates_used contains the names of all of the provisioning templates
kickstart used during a host build. This allows users to be able to
show the name of a provisioning template during PXEBoot, for example
Copy link
Member

@ares ares left a comment

Choose a reason for hiding this comment

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

Thanks @dLobatog, works well. I'll merge shortly, would you mind describing it at TemplateWriting wiki?

@ares
Copy link
Member

ares commented Jan 4, 2017

Merged as 22439f7

@ares ares closed this Jan 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants