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

Do not ensure plugin_parent_dir to be a directory (#148) #149

Closed
wants to merge 2 commits into from

Conversation

turb
Copy link

@turb turb commented Jul 10, 2014

when dealing about plugin installation.
Firstly, it is not plugin.pp business, but more importantly, if this directory is a symlink, it may break the whole installation.

See #148

when dealing about plugin installation.
Firstly, it is not plugin.pp business,
but more importantly, if this directory is a symlink,
it may break the whole installation.
@jenkinsadmin
Copy link

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@dbeckham
Copy link
Contributor

These two commits merge and test properly on the current master. Is there a philosophical reason why the plugin class has this behavior that keeps this from being merged?

Two issues are being corrected here:

  • This module has a hard requirement that /var/lib/jenkins must be a directory. This means that the location and storage space of your Jenkins home can only be managed through mounted partitions instead of symbolic links.
  • It's conceptually improper for a subclass like jenkins::plugins to manage the Jenkins home directory in any way. At the very least, this should be managed from the base class, or a completely separate class specific to installation requirements. In addition, there is absolutely no reason why managing the Jenkins home can't be left up to the operator through some other means.

dbeckham added a commit to dealnews/puppet-jenkins that referenced this pull request Jul 30, 2014
The management of the basic dependencies of a Jenkins plugin should not
be unnecessarily forced on the user.

This change adds a new boolean parameter, `$manage_dependencies`, to
allow the user to specify whether or not jenkins::plugin will create
these dependencies. The default is true to support backwards
compatibility.

When set to false, the `/var/lib/jenkins` directory, the jenkins user
and the jenkins group are expected to be defined elsewhere or already
exist.

In addition, the plugin_parent_dir resource is checked for a previously
defined resource before being created in the same way that the
plugin_dir, user and group are checked.

Specific tests were created for the new parameter and they verify that
plugin_dir is created whether `$manage_dependencies` is true or false.

This fixes voxpupuli#148 in a more backwards compatible way than voxpupuli#149.
dbeckham added a commit to dealnews/puppet-jenkins that referenced this pull request Jul 31, 2014
The management of the basic dependencies of a Jenkins plugin should not
be unnecessarily forced on the user. This change wraps the plugin_parent_dir
resource creation in a `defined` function to allow the user to create the
Jenkins home directory themselves if necessary.

This fixes voxpupuli#148 in a more backwards compatible way than voxpupuli#149.
@jchristi
Copy link
Contributor

Perhaps you could change the ensure => directory attribute to ensure => present instead?

@turb
Copy link
Author

turb commented Aug 22, 2014

@jchristi see https://docs.puppetlabs.com/references/latest/type.html#file-attribute-ensure

present will accept any form of file existence, and will create a normal file if the file is missing.

This means it will create a file, and not a directory, if nothing exists. If we want to keep this clause (and we don't), why have it wrong?

A clause saying "ok, if there is notings, create a directory, if not, do nothing" would work, be does not exist in Puppet.

@PierreR
Copy link

PierreR commented Sep 10, 2014

Any update ?

This has been proposed 2 months ago. No objection has been given. What's going on ?

rtyler pushed a commit to rtyler/puppet-jenkins that referenced this pull request Sep 24, 2014
The management of the basic dependencies of a Jenkins plugin should not
be unnecessarily forced on the user. This change wraps the plugin_parent_dir
resource creation in a `defined` function to allow the user to create the
Jenkins home directory themselves if necessary.

This fixes voxpupuli#148 in a more backwards compatible way than voxpupuli#149.
@rtyler
Copy link

rtyler commented Sep 25, 2014

I'm pretty sure this can be considered resolved at this point

@rtyler rtyler closed this Sep 25, 2014
@rtyler rtyler added this to the 1.2.0: Nestor milestone Sep 25, 2014
@turb
Copy link
Author

turb commented Sep 29, 2014

@rtyler no, it is not.

If I migrate to jenkinsci version of the module, my /var/lib/jenkins symbolic link is deleted, replaced by a new one.

defined(File[$plugin_parent_dir]) does not check if the file exists, it checks if the rule has yet been aplied.

@rtyler
Copy link

rtyler commented Sep 29, 2014

@turb perhaps an acceptable workaround would be to have the Puppet code using the jenkins module declare that file resource? E.g.

file { '/var/lib/jenkins':
   ensure => link,
   target => '/mnt/jenkins',
}

@turb
Copy link
Author

turb commented Sep 29, 2014

@rtyler I did not understood the intention behind this modification. In fact we did not manage this file with Puppet; now we do.

And yes, it does work for us.

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.

6 participants