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

Only create config_dir in specific cases. #210

Merged
merged 1 commit into from
Jun 2, 2016

Conversation

pmlee
Copy link

@pmlee pmlee commented Jan 5, 2016

8adbf88 began managing the $config_dir, but that is only necessary if the default value isn't used or if the default is used AND the OS is gentoo. we ran into a an issue where the $config_dir is being managed by another module, and we therefore had duplicate resources. this change addresses that since in all other cases the $config_dir either exists by default (e.g., FreeBSD) or is created when the haproxy package is installed (seemingly every OS except Gentoo).

//cc @jewjitsu

@pmlee pmlee force-pushed the create_config_dir branch from e8a345c to 4aae7d9 Compare January 5, 2016 17:51
# $config_dir should only be created if default value is used and the OS
# is Gentoo or if a custom value is used
if $config_dir {
if $config_dir == $haproxy::config_dir and $::osfamily == 'Gentoo' {
Copy link

Choose a reason for hiding this comment

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

Shouldn't this comparison be if $config_dir == $haproxy::params::config_dir ...? The value of $config_dir in this manifest is passed down from haproxy (init.pp), through haproxy::instance. The values of $config_dir and $haproxy::config_dir will always be the same, no?

@DavidS
Copy link

DavidS commented Jan 7, 2016

In addition to @antaflos' comments on the code, I'd also ask which other module has authority on touching haproxy's config_dir, and whether it would be better to fix THAT, instead of burdening haproxy with conditionals.

@pmlee
Copy link
Author

pmlee commented Jan 7, 2016

D~

Dependency cycle was detected with the ‘zleslie/pkgng’, ‘0.2.5’ package:

Error: Could not apply complete catalog: Found 1 dependency cycle:

(Exec[pkg update] => Class[Pkgng] => Package[haproxy] => Haproxy::Install[haproxy] => Haproxy::Config[haproxy] => File[/usr/local/etc] => File[/usr/local/etc/pkg.conf] => Exec[pkg update])

M

From: David Schmitt [mailto:notifications@github.com]
Sent: Thursday, January 07, 2016 9:45 AM
To: puppetlabs/puppetlabs-haproxy puppetlabs-haproxy@noreply.github.com
Cc: Mike Lee pmlee@fxcm.com
Subject: Re: [puppetlabs-haproxy] Only create config_dir in specific cases. (#210)

In addition to @antafloshttps://github.com/antaflos' comments on the code, I'd also ask which other module has authority on touching haproxy's config_dir, and whether it would be better to fix THAT, instead of burdening haproxy with conditionals.


Reply to this email directly or view it on GitHubhttps://github.com//pull/210#issuecomment-169683175.

@DavidS
Copy link

DavidS commented Jan 7, 2016

This is indeed a messy situation, with no clear solution. One could point blame to the overzealous Haproxy::Install -> Haproxy::Config dependency and extract the File[$config_dir] into a separate class that is outside of this dependency. Long-term this would suit itself very well to a freebsd-base module.

Short-term special-casing it to $osfamily == 'freebsd' might be a easier solution to contain this here.

@sethlyons
Copy link

the problem really seems to be that there are only two scenarios where the $config_dir needs to be created:

  1. the default $config_dir on Gentoo (since it's not created during package installation)
  2. a non-default $config_dir is used

this isn't a freeebsd-specific problem. in every OS except gentoo, the default $config_dir already exists, so as @DavidS says above, the haproxy module is overzelaous in managing this resource since there should be no need to do so if the default is used.

if the defeault $config_dir was created during installation in gentoo, we could just say:

if $config_dir != $haproxy::params::config_dir {
  file { $config_dir:
    ensure => directory,
    owner  => 'root',
    group  => 'root',
    mode   => '0755',
}

but @pmlee wrote it this way so he didn't screw over the gentoo users.

for completeness sake, the underlying issue is that pkgng manages the file resource /usr/local/etc/pkg.conf. and since haproxy started managing /usr/local/etc, the haproxy file resource began auto-requiring the pkgng file resource, creating the dependency loop.

@jonnytdevops
Copy link

Would it not be simplier to just create a boolean property called something like "manage_conf_directory" and let the user decide?

Thanks

@bmjen
Copy link

bmjen commented Feb 4, 2016

Hi @pmlee, Any updates on this PR?

@sethlyons
Copy link

@pmlee's change fixes the current issue while leaving functionality intact for non-freebsd users. would you still rather us change what he did to what @bmjen requested?

@bmjen
Copy link

bmjen commented Mar 22, 2016

@sethlyons @pmlee Some very valid points were brought up regarding the directory management issues in this define. I believe if we were to continue with this patch, the update made by @antaflos needs to be implemented. Also, another way to make future maintenance of this conditional easier would be to do the check upfront and assign it to a new variable.

Like this:

if $config_dir == $haproxy::params::config_dir and $::osfamily == 'Gentoo' {
  $_manage_config_dir = true
} else {
  $_manage_config_dir = false
}

Then in the lines you changed it can merely say:

if $_manage_config_dir {
  ...
}

I feel like this would make maintenance simpler if we were ever to have to change the conditions for config dir management down the road. Definitely up for discussion though.

@jonnytdevops
Copy link

@pmlee I was just wondering if you had had a chance to look at this? I think this would be very beneficial to the users of this module!

Thanks!

@sethlyons sethlyons force-pushed the create_config_dir branch from 4aae7d9 to c5e2c4e Compare May 24, 2016 18:58
@sethlyons
Copy link

what do you think of this new commit? i believe it should address everyone's concerns.

$custom_fragment = undef,
$config_dir = $haproxy::params::config_dir,
$config_file = $haproxy::params::config_file,
$manage_config_dir = $haproxy::params::manage_config_dir
Copy link

Choose a reason for hiding this comment

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

missing a comma at the end of line here

@DavidS
Copy link

DavidS commented May 24, 2016

I like it, except for the syntax error.

@sethlyons sethlyons force-pushed the create_config_dir branch from c5e2c4e to 41f5e15 Compare May 24, 2016 21:46
@sethlyons
Copy link

updated.

@bmjen bmjen merged commit 6d68d79 into puppetlabs:master Jun 2, 2016
cegeka-jenkins pushed a commit to cegeka/puppet-haproxy that referenced this pull request Dec 19, 2017
Only create config_dir in specific cases.
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.

7 participants