[Modules 4746] Creates class for managing Apache mod_jk connector

[EmersonPrado]

Current state: includes mod_jk module, creates jk.load, jk.conf and worker properties files and adds some description in the README file.

Lacks (as far as my creativity goes): parameter validation, proper parsing of some parameters, worker map file management and tests.

[eputnam]

@EmersonPrado is there anything specifically we can help with on this PR?

[EmersonPrado]

@eputnam Yes: with the tests. I have way too little knowledge on rspec-puppet, so I'm getting lost. I'd really appreciate some help making decent tests which pass.

[eputnam]

the inheritance should fix the broken test. as for the tests, they look fine. one thing to add might be a debian scenario just because things can change across platforms for mods. other than that, i think you're fine.

[eputnam]

there's not really a great pattern to open up access to the vars you reference in the parameters here, so i'll take a page from many of the other mods and say use inheritance here.

) inherits apache {

then you can remove the include ::apache below as well.

[EmersonPrado]

Does that mean that, by using inheritance in the class, the external variables (from ::apache) will be available in the spec test?

[eputnam]

it's going to work just like the include but with a guaranteed parse order. i'm not actually sure what you're looking for, did you have something specific in mind?

[EmersonPrado]

What I exactly need is the mod/jk spec test to access the variables from ::apache class. If I understood you correctly, inheritance would make sure the variables get included before needed in the spec test. Is that correct?

I'm avoiding inheritance here because it's considered an anti-pattern by Puppet itself - see block "Aside: When to Inherit" here.

An alternative would be to declare the parameter as undef in the class signature, then assingn values depending on such parameter, like I did with workers_file and mount_file. So, by assigning something in the test in :parameters section, maybe the required variables will be there. Doesn't seem much clear (I'm in a hurry now), but feel free to ask.

Thanks!

[eputnam]

i commented above, i think the undef pattern you mention would work best, then we can avoid inheritance altogether. as for mocking apache params, you can use "pre_conditions" in a compile block for one:

puppetlabs-apache/spec/classes/mod/auth_kerb_spec.rb

Lines 77 to 105 in 40c1ad4

	context "overriding mod_packages" do
	context "on a RedHat OS", :compile do
	let :facts do
	{
	:id => 'root',
	:kernel => 'Linux',
	:osfamily => 'RedHat',
	:operatingsystem => 'RedHat',
	:operatingsystemrelease => '6',
	:path => '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin',
	:concat_basedir => '/dne',
	:is_pe => false,
	}
	end
	let :pre_condition do
	<<-EOS
	include apache::params
	class { 'apache':
	mod_packages => merge($::apache::params::mod_packages, {
	'auth_kerb' => 'httpd24-mod_auth_kerb',
	})
	}
	EOS
	end
	it { is_expected.to contain_apache__mod("auth_kerb") }
	it { is_expected.to contain_package("httpd24-mod_auth_kerb") }
	it { is_expected.to_not contain_package("mod_auth_kerb") }
	end
	end

is that maybe like what you're thinking off?

[EmersonPrado]

OK. I used the undef pattern for ::apache::logroot only, including a new parameter for it in mod::jk class. Then I changed the file parameters to only the name, as you suggested (so the user can override either filename or diretory at will). Finally, added the logroot parameter in the :params section in the spec test. Let's hope it works!

As for pre_conditions section, I still don't get quite well how it works. I'll try something later. But thanks for the hint!

[eputnam]

parens here for :facts

[eputnam]

in the case of these logroot dependencies, you could just set these to the file's name and then prepend the logroot path below. this would be a more predictable pattern for those who are wanting to override anyway.

[eputnam]

@EmersonPrado,

for logroot, you don't need a $logroot parameter in your class, you should just be able to get it from ::apache, and it will never be undef so you don't need that check either. unless of course you're expecting the user to set a special logroot for the jk mod. does that make sense?

so:

1. get rid of the $logroot parameter (don't forget to remove it from the spec test)
2. you don't need to handle undef values for $apache::logroot, just set shm_file and log_file, like you are without the conditional:

$shm_path = "${::apache::logroot}/${shm_file}"
$log_path = "${::apache::logroot}/${log_file}"

and for future reference, :params in the spec test expects a hash, so it might look like this instead:

let (:params) do
      { :logroot => '/var/log/httpd' }
end

was any of that helpful?

[EmersonPrado]

@eputnam Yes, I'm including the possibility for the user to override logroot directory. I don't know how much this will be used - the user would probably be safer overriding this in ::apache class instead - but I'm leaving this here for pickiness. Maybe it's something to think twice.
As for the undef test, unfortunately the spec test fails without that apparently unnecessary pattern. If I just have $logroot = $::apache::logroot in apache::mod::jk class signature, the spec test will complain there's no $::apache::logroot. This is kinda OK, since the spec test doesn't parse included classes, but it should ignore the external variable, since it's overridden with let (:params) do { :logroot => '/var/log/apache2' } end. But this is a problem somewhere else.

Your correction about spec :params section nailed it.

With last commits, all spec tests pass. I included a Debian context too. So I guess we're all set now.

Thanks a lot!

[eputnam]

@EmersonPrado thanks this is great.