search

SumoLogic / sumologic-collector-puppet-module

Puppet module for installing Sumo Logic's collector.
Other
11 stars 22 forks source link

Unnest module so it will work with Librarian Puppet #13

Closed mcasper closed 8 years ago

mcasper commented 8 years ago

Librarian Puppet expects the root of the repository to be the root of the module.

This has #10 merged in, so should be merged after it gets in.

TerribleDev commented 8 years ago

:-1: @mcasper Just set the path to it in librarian

mod 'sumo-sumo', :git => "https://github.com/SumoLogic/sumo-collector-puppet-module.git",
  :path => "sumo"

I use this with librarian no problem.

mcasper commented 8 years ago

That's a valid workaround, but wouldn't we want this repo to Just Work™?

TerribleDev commented 8 years ago

Everyone's definition of just work is different. IMO this does just work since I can configure librarian to use it, without having to roll a custom version of librarian.

we could also just say...here is an example of using librarian with this module in the readme, thus not polluting the root with many files.

IMO we should keep the root clear so we can add additional folders like rspec, and server spec tests, things that you would not want inside your module.

mcasper commented 8 years ago

That's fair, everyone' definition is different. Although I wouldn't argue that it just works right now, with either the unnesting change or a clear example in the README I might concede that.

IMO we should keep the root clear

In my opinion, we're not cluttering anything with this change. We're just turning 1 directory into 3 (at the moment, I recognize there could be couple more) and eliminating the need for one more irregularity in how this gets installed.

so we can add additional folders like rspec, and server spec tests, things that you would not want inside your module.

rspec-puppet advertises and expects your spec/ directory to be inside your module.

TerribleDev commented 8 years ago

rspec-puppet advertises and expects your spec/ directory to be inside your module.

this is true, that is why we have task engines like rake that can drop files in proper directories when testing things. Keeping them around for all to have inflates things. I get so frustrated when I download various modules, npm packages, gems, etc where tests are included. When you have hundreds of these dependencies you need to download, pack, and perhaps transmit back over the wire to production servers the bytes add up quick. I know I'm kind of alone on this thought process, but some of the things I work on require loads of packages. I know we have wasted tons of time and bandwidth downloading files we will never use, from package authors that don't take such things into consideration.

mcasper commented 8 years ago

True, but you should only have to download the entire module once onto any server, Puppet won't include the tests in the compiled manifests (if you're using a master/agent Puppet setup).

Anyways, I would still argue that the gains in development outweigh the cost of a slightly larger clone.

duchatran commented 8 years ago

@tparnell8, @mcasper : Thank you both for contributing to the repo and the discussion. Taking the cue from our Chef repo (https://github.com/SumoLogic/sumologic-collector-chef-cookbook) and some other Puppet repos, I think it's fine to merge this PR.
Tommy: As for your point on having test modules, I got it. However, we received requests from other customers to include tests in our repos as they are increasingly relying on them in their build process. So I don't think we can avoid that (and sorry for not solving your pain here). This takes me to the next point: we don't have any test right now for this repo. If you guys have built some, they could be some great contribute.

mcasper commented 8 years ago

@duchatran Definitely, I'd be glad to contribute some tests