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

Correct sites-enabled handling in Debian // Remove trailing white-spaces where applicable #51

Merged
merged 4 commits into from Apr 2, 2014

Conversation

Projects
None yet
2 participants
@raoulbhatia
Copy link
Contributor

raoulbhatia commented Mar 27, 2014

  1. Correct sites-enabled handling in Debian
  2. Remove trailing white-spaces where applicable

(fixed tests - sorry for the noise)

@raoulbhatia raoulbhatia changed the title Correct sites-enabled handling in Debian // Remove trailing white-spaces Correct sites-enabled handling in Debian // Remove trailing white-spaces where applicable Mar 27, 2014

@raoulbhatia

This comment has been minimized.

Copy link
Contributor

raoulbhatia commented Mar 27, 2014

i finally managed to fix my pull request - however, there seems to be an error with travis as the build using ruby 1.8.7 fails with an error which does not seem to be related to my pull request.

@raoulbhatia

This comment has been minimized.

Copy link
Contributor

raoulbhatia commented Mar 27, 2014

Fix .gemfile as described in travis-ci/travis-ci#2112 .

Please let me know if you will accept this pull request and i will then prepare a couple of others which are hanging in my queue :)

@javierbertoli

This comment has been minimized.

Copy link
Member

javierbertoli commented Mar 30, 2014

Hi, sorry for not answering before. Surely will add whatever you have to contribute :)

BTW, great catch this travis issue, it was bugging me, and had no time to check what it was.

@@ -311,6 +311,11 @@
default => "${nginx::config_dir}/conf.d",
}

$vedir = $::operatingsystem ? {
/(?i:Ubuntu|Debian|Mint)/ => "${nginx::config_dir}/sites-enabled",

This comment has been minimized.

@javierbertoli

javierbertoli Mar 30, 2014

Member

I 👍 separating this into a variable, but just to make a clear distinction from $vdir, I would suggest renaming this var to $v_enable_dir or something like that?

This comment has been minimized.

@raoulbhatia

raoulbhatia Mar 31, 2014

Contributor

I am choosing $vdir_enabled to make it easier to grep for $vdir.* to catch all related lines of code.

@@ -427,7 +432,7 @@

if $nginx::config_file_default_purge {
$default_site = $::operatingsystem ? {
/(?i:Debian|Ubuntu|Mint)/ => [ "${nginx::config_dir}/sites-enabled/default", "${nginx::config_dir}/sites-available/default" ],
/(?i:Debian|Ubuntu|Mint)/ => [ "${nginx::vedir}/default" ],

This comment has been minimized.

@javierbertoli

javierbertoli Mar 30, 2014

Member

When purging the configuration, you should remove both dirs, sites-available and site-enabled.

This comment has been minimized.

@raoulbhatia

raoulbhatia Mar 31, 2014

Contributor

As my changes will compile an nginx.conf file which includes [...]/conf.d/*conf and [...]/sites-enabled/* only, removing [...]/sites-enabled/default should be sufficient.

Also, as far as I have been using and have seen others use xyz-{available,enabled} directories:

  1. Store all possible configurations in xyz-available and
  2. enable only the ones you need via syz-enabled.

e.g. if you run apache's a2dissite or a2dismod, only the {sites,mods}-enabled symlink is removed but not the whole configuration. The same applies to (newer) php, lighttpd, ... configuration (on Debian/Ubuntu).

What are your thoughts on this?

This comment has been minimized.

@javierbertoli

javierbertoli Mar 31, 2014

Member

My concern is that the way you propose will give the module different behaviour
depending on the underlaying OS: on Debian and derivatives it would only disable
the vhost without removing it, while on RH and derivatives it will be
effectively removing all traces of the default vhost.

The idea suggested by the variable is that we purge the "default" config file,
so I expect, by it's name, to remove it completely from nginx's config dir.

Just disabling it should be achieved with

nginx::vhost { 'default': ensure => disabled }

as with any other vhost management.

Perhaps adding another variable $config_file_default_disable would be a
possibility, but it would mean to also keep the file on RH's related (and
renaming it), and it's possible to achieve the same result in a cleaner way as
stated before.

I prefer the current behaviour for this variable (removing both files), for the
sake of coherence across OSes.

Al other changes from this PR are OK with me.

@raoulbhatia

This comment has been minimized.

Copy link
Contributor

raoulbhatia commented Mar 30, 2014

On 30 March 2014 21:44:16 CEST, "Javier Bértoli" notifications@github.com wrote:

Hi, sorry for not answering before. Surely will add whatever you have
to contribute :)

BTW, great catch this travis issue, it was bugging me, and had no time
to check what it was.


Reply to this email directly or view it on GitHub:
#51 (comment)

Thanks for gettting back to me.

I have made several enhancements that i needed in my setup which are based on one another.
Please see my repository for details.

I would create one pull request after the other so that you can review and merge them.

So the next pull request will be opened when the first one is updated and merged.

I will be able to address your comments in the next days. Of course, if you have any further comments, please let me know.

Cheers,

Raoul

Raoul Bhatia

@javierbertoli

This comment has been minimized.

Copy link
Member

javierbertoli commented Mar 30, 2014

Sure thing. If you can address these changes I sugested a few minutes back, I'll merge the PR. If not let me know, and I'll try to fix it myself.

@raoulbhatia

This comment has been minimized.

Copy link
Contributor

raoulbhatia commented Mar 31, 2014

I've updated my pull request to incorporate your suggestions and also to resemble puppetlabs-apache's way of handling the sites-enabled directory.

Feedback appreciated!

Thanks,
Raoul

@raoulbhatia

This comment has been minimized.

Copy link
Contributor

raoulbhatia commented Mar 31, 2014

Please correct me if I am wrong: To be able to use nginx::vhost { 'default': ensure => disabled }, one would need to implement proper ensure => deleted handling?

Meaning: This does not work right now/out of the box?

@javierbertoli

This comment has been minimized.

Copy link
Member

javierbertoli commented Mar 31, 2014

You are still not purging both files with $config_file_default_purge, as I suggested, therefore leaving different behaviour between the OSes.

Regarding the usage of nginx::vhost, ensure => disabled or enable => false would both have the same effect of removing the $vdir/default instance. ensure is a metaparameter, so it's automatically added to all resources in Puppet and will work out-of-the-box.

@raoulbhatia

This comment has been minimized.

Copy link
Contributor

raoulbhatia commented Apr 2, 2014

Hi!

In my tests, both files got removed. Please see raoulbhatia@f8dd494#diff-60ae41fd0a31977447947f59940ee9a4R433

Running "puppet agent --test --noop":
Notice: /Stage[main]/Nginx/File[/etc/nginx/sites-available/default]/ensure: current_value file, should be absent (noop)
Info: /Stage[main]/Nginx/File[/etc/nginx/sites-available/default]: Scheduling refresh of Service[nginx]
Notice: /Stage[main]/Nginx/File[/etc/nginx/sites-enabled/default]/ensure: current_value link, should be absent (noop)
Info: /Stage[main]/Nginx/File[/etc/nginx/sites-enabled/default]: Scheduling refresh of Service[nginx]

Maybe i do not understand what the issue is that you're trying to point out?

Thanks,
Raoul

javierbertoli added a commit that referenced this pull request Apr 2, 2014

Merge pull request #51 from raoulbhatia/master
Correct sites-enabled handling in Debian // Remove trailing white-spaces where applicable

@javierbertoli javierbertoli merged commit 6d3b299 into netmanagers:master Apr 2, 2014

1 check passed

default The Travis CI build passed
Details
@javierbertoli

This comment has been minimized.

Copy link
Member

javierbertoli commented Apr 2, 2014

No, my bad. It's all OK then, merging. Thanks for all your help. Any other PRs will be welcomed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment