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

Add confd_only option #878

Merged
merged 1 commit into from
Oct 17, 2016
Merged

Add confd_only option #878

merged 1 commit into from
Oct 17, 2016

Conversation

wyardley
Copy link
Collaborator

@wyardley wyardley commented Sep 20, 2016

This PR adds an option $confd_only (defaulting to false), that avoids using the Ubuntu style $conf_dir/sites-{available,enabled} mechanism with symlinks. Obviously, the current way works, but I don't personally prefer it; also, presumably one is using Puppet to manage configs in a lot of scenarios, so on platforms where this convention isn't used, or when managing all of the nginx configs through Puppet, the symlinks don't really gain you much advantage.

A couple notes:

  1. It just doesn't force the resource if the param is set, so $conf_dir/sites-{available,enabled} won't be forced to be present or absent
  2. To purge, with $confd_only set, both $vhost_purge and $confd_purge must be set, otherwise, it will silently not purge (I can make it fail in that case if folks think it's cleaner).
  3. With the option set, the sites-enabled dir will also not be included in the main nginx.conf

I can't tell where the main configurable params of the module are documented; the resources have their parameters documented, but init.pp and config.pp don't seem to have the main params documented. But the caveat above should probably be documented somewhere.

Happy for suggestions of improvements, or about making the style more consistent (I purposely didn't use the if $foo == truestyle used elsewhere in the module for things that are already verified as booleans, though I didn't change existing cases) or improve test coverage further.

@wyardley
Copy link
Collaborator Author

wyardley commented Sep 20, 2016

Also, if folks think it's better, I could try to consolidate the logic in these two spots, if someone can suggest a better way or place to do this.
https://github.com/voxpupuli/puppet-nginx/pull/878/files#diff-3bfa339c375e139166a7afc6cb8c5b62R360
https://github.com/voxpupuli/puppet-nginx/pull/878/files#diff-a43305b17067171ae7b2f2ebefd53622R541

Additionally, if there's a better way to have the precondition spec/defines/resource_vhost_spec.rb:46 set nginx::config in a way that follows https://github.com/voxpupuli/puppet-nginx/blob/master/docs/hiera.md, can someone make a suggestion or send an example of somewhere in the tests that this is being done?

@wyardley
Copy link
Collaborator Author

Rebased to handle a couple conflicts, and also updated it to handle conf.stream.d

@wyardley
Copy link
Collaborator Author

I also added some additional tests related to streams here.
I would love suggestions on how to make this code a little cleaner... had thought about setting a variable (in params or config) for some of these directories, but because of all the logic involved, I'm not sure how much that would really streamline things.

context 'when vhost_purge false' do
let(:params) { { vhost_purge: false } }
let(:params) { { vhost_purge: false, stream: true } }
Copy link
Member

Choose a reason for hiding this comment

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

please convert to do/end block (because there are multiple attributes in the hash.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just got rid of $stream since it didn't look like it's actually doing anything useful

end

context 'when stream true and confd_only true' do
let(:params) { { stream: true, confd_only: true } }
Copy link
Member

Choose a reason for hiding this comment

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

same here, do/end please.

@wyardley
Copy link
Collaborator Author

@bastelfreak: the recent changes (removing $stream variable) should resolve the feedback above.
Tests seem to be passing except for that weird Travis problem.

@wyardley wyardley closed this Oct 11, 2016
@wyardley wyardley reopened this Oct 11, 2016
@wyardley
Copy link
Collaborator Author

Ran acceptance tests (CentOS 6, Ubuntu 14) and look Ok. Closed / reopened just to see if Travis is any less cranky now.

@dhoppe
Copy link
Member

dhoppe commented Oct 11, 2016

@wyardley You do not need to close / reopen the PR to trigger a build at TravisCI. Just login at TravisCI via GitHub and push the "Restart build" button.

But it looks like a timeout at:

Facter::Util::Fact
  neither nginx or openresty in path
    should eq nil
  nginx
    with current version output format

@wyardley
Copy link
Collaborator Author

wyardley commented Oct 11, 2016

@dhoppe: Didn't know I had permission to restart jobs.
I saw that timeout too, it's from @petems's changes for #913.
But if I set PUPPET_VERSION="~> 4.0" (and I'm using Ruby 2.3.1p112 already), I don't see that behavior, and the same test also seems to pass with the other Travis options... so I'm stumped as to why it's causing a timeout at that spot (unless one of the calls isn't actually stubbed? I looked at the code and didn't see any obvious problems in that regard).

@dhoppe
Copy link
Member

dhoppe commented Oct 11, 2016

@wyardley Take a look at #918. It looks like this is some sort of issue with mocha version 1.2.0. @petems already created an issue freerange/mocha#272.

@wyardley
Copy link
Collaborator Author

Also, does it seem to make sense to have $vhost_purge affect the streams files as well? Or should it be limited only to vhosts?

@bastelfreak: the changes you requested should be resolved now. Would appreciate any other discussion about this feature, or any reservations folks have about the premise of it.

Streams functionality is still broken; it may make sense to re-enable the $streams parameter, but it would need to go along with fixing it (currently, the files aren't even included). So if this is merged, I can happily make a new PR to replace / update the existing ones for fixing that.. #790, #782, etc.

# Err on the side of caution - make sure *both* $vhost_purge and
# $confd_purge are set if $confd_only is set, before purging files
# ${conf_dir}/conf.d
if (($confd_only and $vhost_purge) or !$confd_only) {
Copy link
Member

Choose a reason for hiding this comment

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

note to myself: we really need a styleguide for this

@bastelfreak bastelfreak merged commit 3e9ea24 into voxpupuli:master Oct 17, 2016
@wyardley wyardley mentioned this pull request Oct 18, 2016
@wyardley wyardley deleted the feature_conf_d_only branch October 27, 2016 17:16
cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Sep 13, 2019
Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants