over-specification of dependencies

[mc0e]

This is a module for gitlab. You shouldn't be specifying the loading of more packages outside that scope. Eg most puppet configurations will already have something to load openssh-server, and very likely postfix, curl, etc, etc. By specifying Package instantiations for these you make your module incompatible with others.

For servers which don't have much else on them, what you are doing with dependencies has some value, especially given that there's no .deb and .rpm available for gitlab, which would otherwise be a preferred way to handle the dependencies. I think it should be left to the user of your module though to choose whether to load this class or not.

Besides packages, gitlab::pre class sets up a user, and on redhat at least it sets up a home directory for gitlab (I think this is just permissions?). The user stuff should be done by your module. The package pre-requisites should be separated out into an optional module though. That package prerequisites module can set up the dependency relationship with modules which need to be loaded afterwards.

Being so little left in the gitlab::pre class, it might be simpler to remove it and put the user bit into the gitlab::server class.

[zebpalmer]

I disagree. The point of puppet is to manage configuration from the ground up. Sure, many of us will have base classes/modules setting up some of these things already but where does one draw that line? Most of us have git already installed, should this module not install it? a gitlab module without git would be pretty useless, as would one without openssh-server. I would expect any decent puppet module to be able to take a fresh install and bootstrap the target service including all non-standard deps. Openssh-server and postfix are both optional packages therefore they should be installed, especially ssh. One could argue that the module should allow one to set the smtp server in the config/environments/production.rb file, but it seems the gitlab assumed install is to use postfix on localhost. Regardless, since it's not pushing configuration for these deps, it's not causing any conflicts in my environment.

The only issue I have with the pre.pp is it's stopping iptables, but that's only on rhel boxes which I don't have.

[mc0e]

Ok, so there is a question about what the scope of the module should be. it could be for installing a gitlab service, which is usual for modules in puppet forge, or it could be for configuring a gitlab service.

I'm certainly not arguing against distribution of puppet code for configuring a whole gitlab server "from the ground up", but it's not necessarily best to do that in a single module. I'd argue that modules should take a modular approach rather than a monolithic one.

[preflightsiren]

yep, agree with mc0e; This module is a pain to debug, purely from the point that it does more than install and configure gitlab..

Some suggestions on where to start would be to move the apt/nginx configuration out, and use separate modules, either sbadia/apt if you want to make your own, or look at existing modules such as puppetlabs/apt and https://github.com/jfryman/puppet-nginx

[sbadia]

I take care of it this weekend (with the transition to gitlab 5.2)

[atomaka]

Was there a reason you decided not to use puppetlabs-ruby to manage ruby versions?

[sbadia]

No, I haven't pushed, I wanted to test ubuntu (and centos maybe) before. :-)

[atomaka]

I ran into the specific example that @mc0e cited today. By defining package ['openssh-server'], we lose the ability to manage ssh through other available modules. I originally didn't understand why @mc0e was recommending a third module but it makes much more sense now that I realize that a package resource cannot be redefined. If only puppet was fine when duplicate package resources matched completely.

This would make the three parts of this project: 1) puppet-gitlab: Strictly related to the service. Should setup the user, clone the source, and do configurations as necessary. Should contain no, or nearly no, package definitions. 2) puppet-gitlab-service: Installs dependencies through specific modules and packages and then uses the puppet-gitlab module to install GitLab. 3) vagrant-gitlab: uses puppet-gitlab-service to install puppet on vagrant.

[sbadia]

Hi, sorry for the delay… (and sorry for my poor english…). Yes indeed, I did not finish the work on this. I am not against this change, it also joined the #47 : in a production environment it does not work if puppet-gitlab is not in a clean env. (at the beginning i made this module for "one service = one virtual machine"). By cons the dependencies should be changed, something like this go?

puppet-gitlab-service or puppet-gitlab-prerequist dependencys:

• puppetlabs/mysql
• puppetlabs/stdlib
• fsalum/redis
• puppetlabs/nginx
• puppetlabs/ruby
• sbadia/gitlab
• puppetlabs/git ?
• saz/ssh ?

puppet-gitlab dependencys:

• nothing

vagrant-gitlab deps:

• puppet-gitlab-service

[atomaka]

I think it might be tough to remove all dependencies from puppet-gitlab. A quick example of this is setting up the gitlab database within the database of choice. This still probably belongs in puppet-gitlab and will force a dependency on puppetlabs/mysql. The important part is just that packages that we use but are not managing should not have a package resource call in puppet-gitlab to avoid conflicts.

[sbadia]

A good compromise would be to simply use the defined function (as suggested in https://github.com/ubc/puppet-gitlab/commit/9c4659214a5f6871eb93b9e7b4f6b1aab1865c9d by Pan Luo). This avoids making too complicated things with module inter-dependencies… What do you think about ?

[mc0e]

Using 'defined' is all but deprecated. It's a poor solution. The problem is that you have no idea which order things are going to be loaded in, and can get unpredictable behaviour.

I think there should be a gitlab dependencies module with a bunch of classes that are required by gitlab, and which configure various functionality that gitlab depends on. These modules should have 'enable' as a class parameter, so that (as of pupppet version 3) it can be overridden with hiera without needing to edit the source. The granularity of these classes should match the level at which gitlab resources require these external resources.

So, gitlab requires gitlab_dependencies/ssh_server which (if enabled) sets up ssh.

Similarly I think all the variables in gitlab::params should be class parameters so they can be overriden from hiera.

[sbadia]

Sorry, Indeed you're right (the argument hiera convinces me) thanks !

[atomaka]

@mc0e Can you point me in the direction of a module using this method for loading classes? I would love to see it in action.