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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

add PuppetDB configuration on puppet master side #182

Merged
merged 1 commit into from
Feb 5, 2015
Merged

add PuppetDB configuration on puppet master side #182

merged 1 commit into from
Feb 5, 2015

Conversation

mmoll
Copy link
Contributor

@mmoll mmoll commented Sep 22, 2014

more a WIP for discussion. comments please 馃摪

@ekohl
Copy link
Member

ekohl commented Sep 22, 2014

We've had some discussion about this in the past and while personally I'm not against this, others were. See theforeman/foreman-installer#46 for some history.

You will also need to set the reporting, storeconfigs and storeconfigs_backend if you want full integration.

@mmoll
Copy link
Contributor Author

mmoll commented Sep 22, 2014

There are already options for reports, storeconfigs and storeconfigs_backend, those would have to be set accordingly, or do you want these to be also set automatically when $server_puppetdb_host is set?

Regarding the integration, we also had a short talk on IRC about that, my desire here is to get support for configuring a Puppet master to talk to PuppetDB into our puppet-puppet.

Regarding the possibility of pulling in support to install/configure PuppetDB itself, personally I would love it, as foreman-installer could then also be used to automate away that task in the same way as Foreman (without a Puppet master) and Puppet (master, with connection to Foreman on another host) itself, but that duplicate postgresql include and pulling in puppetlabs-inifile without further usage don't seem to be worth it at the moment.

@ekohl
Copy link
Member

ekohl commented Sep 22, 2014

There are already options for reports, storeconfigs and storeconfigs_backend, those would have to be set accordingly, or do you want these to be also set automatically when $server_puppetdb_host is set?

I don't know what I want ;) Just considering the options at this point.

@mmoll
Copy link
Contributor Author

mmoll commented Sep 22, 2014

I didn't implement it, because it's just easier by leaving it as-is ;)

@domcleal
Copy link
Contributor

Regarding PuppetDB support in the installer generally, I'm less indifferent now and am more keen to include it - particularly as we've gained other optional bits and kafo has improved the installer since the last PR was opened. That said, I think other Foreman developers are less keen to make use of PuppetDB and similar tools than I am.

Regarding this PR specifically, I think it overlaps too much with puppetdb::master::config, which does a better job. The only thing that might be preferable in puppet-puppet is routes.yaml as it's a generic config file, but there aren't many uses for creating and configuring it.

Edit: my conclusion is, do it, but use the official puppetdb module instead of duplicating it, and let's figure out how to make them play nicely together automatically.

@mmoll
Copy link
Contributor Author

mmoll commented Dec 28, 2014

Updated WIP, now using puppetlabs-puppetdb. That seems to work, storeconfig_backend and reports have to be set to puppetdb seperately to use all these, respectively.

Comments?

@ekohl
Copy link
Member

ekohl commented Dec 28, 2014

I wonder if there should be an include puppetdb::server somewhere. You already started to manage puppetdb from this module.

@mmoll
Copy link
Contributor Author

mmoll commented Dec 28, 2014

Not yet. That would be the next step, after managing the Puppet master side.

@mmoll
Copy link
Contributor Author

mmoll commented Jan 24, 2015

rebased

@ekohl
Copy link
Member

ekohl commented Jan 26, 2015

I wouldn't be against this, but I'm hesitant to add it as an unconditional dependency. I'm leaning to doing so because IMHO PuppetDB is a valuable addition in many cases.

@mmoll
Copy link
Contributor Author

mmoll commented Jan 30, 2015

now with added test. no idea what's going on with 2.7 there...

@ekohl
Copy link
Member

ekohl commented Jan 30, 2015

I wonder if a gem broke compatibility with 2.7.

@mmoll
Copy link
Contributor Author

mmoll commented Feb 3, 2015

I messed up metadata.json. All 馃挌 now

@ekohl
Copy link
Member

ekohl commented Feb 3, 2015

Maybe add a test to the 'with no customer parameters' that it should not contain the puppetdb class?

Other than that there's a puppetlabs recommendation that for soft dependencies it should be in README rather than metadata.json. See #162 (comment) as well.

@mmoll
Copy link
Contributor Author

mmoll commented Feb 3, 2015

all done, I think

@ekohl
Copy link
Member

ekohl commented Feb 3, 2015

馃憤 from my side. @domcleal?

@mmoll
Copy link
Contributor Author

mmoll commented Feb 4, 2015

needed a rebase again

class { '::puppet':
server => true,
server_puppetdb_host => 'mypuppetdb.example.com',
server_reports => 'puppetdb',
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick, should we keep 'foreman' in here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As foreman got optional, I presume not, but I wouldn't mind having it also in just as example.

Copy link
Member

Choose a reason for hiding this comment

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

I do think people will be surprised if suddenly they don't have foreman reports anymore so I'd prefer having it in the example.

Copy link
Member

Choose a reason for hiding this comment

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

And just now I see it was already merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sorry. Please feel free to push it straight to the repo, I guess it'd be server_reports => 'puppetdb,foreman'

Copy link
Member

Choose a reason for hiding this comment

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

No problem, it's just a minor comment :)

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in d640db0

@domcleal
Copy link
Contributor

domcleal commented Feb 4, 2015

Otherwise 馃憤

@mmoll
Copy link
Contributor Author

mmoll commented Feb 4, 2015

updated

@domcleal domcleal merged commit ce7bfca into theforeman:master Feb 5, 2015
@domcleal
Copy link
Contributor

domcleal commented Feb 5, 2015

Merged, thanks @mmoll!

@mmoll mmoll deleted the puppetdb branch February 5, 2015 08:44
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.

None yet

3 participants