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/apache name #1070

Merged
merged 2 commits into from
Apr 3, 2015
Merged

Fixes/apache name #1070

merged 2 commits into from
Apr 3, 2015

Conversation

stevenpost
Copy link

Based on the work of @fraenki in #1056
His patch is rebased against a more recent master, and I added 2 patches.

  • fail if the apache base class is not defined
  • ensure the base class is loaded during the tests

This was not the case during the previous versions, which caused some tests to fail after changing the parameter references used.

@stevenpost
Copy link
Author

ok... now I'm lost

In the Travis log:
Puppet::Error:

$concat_basedir not defined. Try running again with pluginsync=true on the [master] and/or [main] section of your node's '/etc/puppet/puppet.conf'. at /home/travis/build/puppetlabs/puppetlabs-apache/spec/fixtures/modules/concat/manifests/setup.pp:20

My local test run was fine.

@stevenpost
Copy link
Author

My Local run was againt another branch... stupid me...

This 'dev' class seems broken to me, it eiter requires apache::package(FreeBSD) or apache, but not both, as that causes a duplicate resource error.

@igalic
Copy link
Contributor

igalic commented Mar 16, 2015

_

@stevenpost
Copy link
Author

Ok, I got it down again to 1 error: FreeBSD
Now that 'apache' is required, I added it to the tests as a precondition.
For FreeBSD however this may cause problems as the apache::dev class requires the apache::package class. This class is created in apache, but that is not enough to get rid of the 'fail()' triggering there. I can't add it as a precondition in the test like it was, as the apache::mpm class defines the apache::package class like a resource, causing it to fail again.

So... I'm stuck now.
This is only a problem for FreeBSD, other osfamilies are not affected.

@stevenpost
Copy link
Author

I'm not all that familiar with FreeBSD, but is that actually needed?
@ptomulik you wrote the if for the apache::dev class, could you shed some light on this?

@ptomulik
Copy link
Contributor

Not 100% sure, but it looks like the purpose of apache::dev on FreeBSD is to just ensure that apache::package was already included. AFAIR there is no separate dev package(s) on FreeBSD so the apache/dev.pp manifest only ensures that normal apache port is installed. Note, that $::apache::params::dev_packages evaluates to undef under FreeBSD.

The installation of apache package(s) is handled quite differently under FreeBSD. You may skim through a comment in init.pp to get some insight. In most systems the list of dev_packages is defined in apache/params.pp and they're installed in apache/dev.pp. On FreeBSD, however, it's apache/mpm.pp which determines what package shall be installed and then uses the apache/package.pp to install appropriate apache package. My intent at that time was to promote this approach also on other system (as it seemed suitable), but as I see, nothing has changed in this matter so far.

@stevenpost
Copy link
Author

I removed the 'fail()' condition for FreeBSD, this allows all tests to pass.
I suspect this also allows the actual run on FreeBSD to complete in more cases, as the order of inclusion matters quite a lot when defining a class as a resource somewhere.

Also, I don't really like how the module is structured so differently between FreeBSD and other systems, to be honest. I think it should be more uniform, either moving the install for FreeBSD back to the init class like the others, or vice versa. Does FreeBSD allow the complete Apache httpd package to be compiled like other systems do? Where the different mpm modules are compiled anyway, and use configuration to load the required module? I guess it would make things simpler.
Also note that Debian started to bundle the mpms into the main apache2 package with 2.4, instead of packaging them separately, still to be loaded as different modules though.

@ptomulik
Copy link
Contributor

Also, I don't really like how the module is structured so differently between FreeBSD and other systems, to be honest.

It was a huge changeset when I was adding the FreeBSD support, and I decided to "not touch" other systems (even if this could unify code). I was almost sure, that it would not be accepted if I altered other OSes. But FreeBSD was really specific...

I think it should be more uniform, either moving the install for FreeBSD back to the init class like the others, or vice versa.

Using special manifest, such as the apache/package.pp for that shouldn't hurt, but may add more flexibility and modularity.

Does FreeBSD allow the complete Apache httpd package to be compiled like other systems do?

Actually, it's a standard approach on FreeBSD -- to compile source packages (ports) instead of just install binary packages from a repository. It was an approach, at least at that time I was struggling with FreeBSD (8.X). I'm not sure how this evolved since that.

Where the different mpm modules are compiled anyway, and use configuration to load the required module?

For apache 2.2 there were separate FreeBSD ports for different MPMs. For example, in order to use ITK MPM, you had to compile and install a port named apache22-itk-mpm. At the same time, you had to modify certain system-wide config file(s) to let the system know what MPM it shall use (/etc/make.conf AFAIR). This was at least the case at that time I worked on the apache module. This changed with apache 2.4, and since 2.4 there was single port providing all the MPMs (one could choose an MPM at compile time or use loadable MPM modules).

@stevenpost
Copy link
Author

We're making progress... this is the second test run that failed in 'strict' due to some fact not being available in the test. Now running the third...
First the 'id' fact, no the 'path' fact, before it was the 'concat_basedir' that was missing.

@cmurphy
Copy link
Contributor

cmurphy commented Mar 26, 2015

@stevenpost could you squash your three commits into one? Thanks!

fraenki and others added 2 commits March 27, 2015 09:58
This also implies the dependency for the tests needed to be changed.
Because of the changed dependency, tests needed some extra facts to please
the concat module.
@stevenpost
Copy link
Author

  • rebased on master
  • squashed 3 last commits into one

@stevenpost
Copy link
Author

Builds seem to fail due to availability problems with Github.

@stevenpost stevenpost closed this Mar 27, 2015
@stevenpost stevenpost reopened this Mar 27, 2015
igalic added a commit that referenced this pull request Apr 3, 2015
@igalic igalic merged commit 69e0a89 into puppetlabs:master Apr 3, 2015
@igalic
Copy link
Contributor

igalic commented Apr 3, 2015

thank you @stevenpost and @fraenki!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants