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

Install all modules before adding custom configs #1221

Merged

Conversation

mpdude
Copy link
Contributor

@mpdude mpdude commented Oct 8, 2015

When setting up a new machine from scratch and adding some ::apache::custom_configs, I see these:

Info: /Stage[main]/Profiles::Apache/Apache::Custom_config[my-permissions-generic]/File[apache_my-permissions-generic]: Scheduling refresh of Exec[service notify for my-permissions-generic]
Info: /Stage[main]/Profiles::Apache/Apache::Custom_config[my-permissions-generic]/File[apache_my-permissions-generic]: Scheduling refresh of Exec[remove my-permissions-generic if invalid]
Notice: /Stage[main]/Profiles::Apache/Apache::Custom_config[my-permissions-generic]/Exec[service notify for my-permissions-generic]/returns: AH00526: Syntax error on line 22 of /etc/apache2/sites-enabled/000-some.site
Notice: /Stage[main]/Profiles::Apache/Apache::Custom_config[my-permissions-generic]/Exec[service notify for my-permissions-generic]/returns: Invalid command 'RewriteEngine', perhaps misspelled or defined by a module not included in the server configuration
Notice: /Stage[main]/Profiles::Apache/Apache::Custom_config[my-permissions-generic]/Exec[service notify for my-permissions-generic]/returns: Action '-t' failed.
Notice: /Stage[main]/Profiles::Apache/Apache::Custom_config[my-permissions-generic]/Exec[service notify for my-permissions-generic]/returns: The Apache error log may have more information.

What happens is that I have already put some custom virtual host in place that requires some module (e.g. mod_rewrite). Then, a custom_config is applied and triggers the config test. That test fails because the module (required by the virtual host) is not yet in place.

@DavidS
Copy link
Contributor

DavidS commented Oct 9, 2015

There is no reason for having Exec[service notify for my-permissions-generic]. That only leads to multiple restarts of apache, when only one is needed. Instead notify Class['apache::service'], which should already have all the proper relationships set up.

@DavidS DavidS closed this Oct 9, 2015
@mpdude
Copy link
Contributor Author

mpdude commented Oct 9, 2015

@DavidS I don't do this. I just do

apache::custom_config { 'my-permissions-generic': ... }

and the module does the rest. The problem is that at the time it does this (and runs the config test, also a feature internal to the module) the Apache modules are not installed yet.

I don't think this issue should be closed yet, could you please re-open it?

@DavidS
Copy link
Contributor

DavidS commented Oct 9, 2015

Sorry, I missed that part. I'll have another look.

@DavidS DavidS reopened this Oct 9, 2015
@DavidS
Copy link
Contributor

DavidS commented Oct 9, 2015

The main problem with your proposed solution would be that the collector <||> will realize all virtual resources matching (see https://docs.puppetlabs.com/puppet/latest/reference/lang_virtual.html#realizing-with-a-collector).

This will kill any configuration relying on virtual apache::mod resources. I've spelunked a bit through the code and I think the best bet is to try to connect the mod packages from https://github.com/puppetlabs/puppetlabs-apache/blob/master/manifests/mod.pp#L70 directly to the verification exec (https://github.com/puppetlabs/puppetlabs-apache/blob/master/manifests/custom_config.pp#L52), probably best done using a tag. If you are in there, please also rename that exec to syntax verification for ..., because - damn - that confused me.

@mpdude
Copy link
Contributor Author

mpdude commented Oct 9, 2015

Not sure I get you right. We should connect not only the package (https://github.com/puppetlabs/puppetlabs-apache/blob/master/manifests/mod.pp#L70) as it might not even be installed. We need something to refer to the ::apache::mod as a whole.

Any could you please give me an example how you would use a tag here?

@mpdude mpdude force-pushed the custom-config-requires-basic-setup branch 2 times, most recently from 249f698 to f0f461a Compare October 9, 2015 13:41
@mpdude
Copy link
Contributor Author

mpdude commented Oct 9, 2015

@DavidS What about the current solution with a shared anchor? Do you think this might work?

@DavidS
Copy link
Contributor

DavidS commented Oct 9, 2015

This looks like a very nice idea! If you could just squash/rebase it into a single commit, please?

@mpdude mpdude force-pushed the custom-config-requires-basic-setup branch 2 times, most recently from d3be1a8 to 13b44ff Compare October 9, 2015 13:49
@mpdude
Copy link
Contributor Author

mpdude commented Oct 9, 2015

Done.

It also passed a quick check and provisioned a machine correctly. But of course, that might also be resource ordering luck 😄 .

@DavidS
Copy link
Contributor

DavidS commented Oct 9, 2015

If jenkins goes green, I'll merge it. You are in the best position to judge whether it solves your problem. I like the change and I'd be stupid to try to second guess you :-)

@mpdude
Copy link
Contributor Author

mpdude commented Oct 9, 2015

Then, as a German, I'll go and port https://github.com/hmlb/phpunit-vw to Puppet.

@DavidS
Copy link
Contributor

DavidS commented Oct 9, 2015

:-D

@mpdude
Copy link
Contributor Author

mpdude commented Oct 9, 2015

Too bad. I hate those tests that fail when you change a natural language string printed to the user.

@DavidS
Copy link
Contributor

DavidS commented Oct 9, 2015

I've yet to find a better way to ascertain that the right error is thrown :-/

The problem is that we have defines (apache::custom_config and apache::mod) on both sides so we cannot easily reference the one side from the other (when we want to avoid the <||> syntax). So, we use an anchor as the reference point and make things happen before or after that.
@mpdude mpdude force-pushed the custom-config-requires-basic-setup branch from 13b44ff to 9d8efec Compare October 9, 2015 15:14
DavidS added a commit that referenced this pull request Oct 9, 2015
Install all modules before adding custom configs
@DavidS DavidS merged commit 22ed027 into puppetlabs:master Oct 9, 2015
@DavidS
Copy link
Contributor

DavidS commented Oct 9, 2015

Danke, daß Du Dir die Mühe gemacht hast das ordentlich umzusetzen!

@mpdude
Copy link
Contributor Author

mpdude commented Oct 9, 2015

Vielen Dank für Deine schnelle Reaktion und den Merge :)

@mpdude mpdude deleted the custom-config-requires-basic-setup branch October 9, 2015 15:50
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.

3 participants