Add params and ini_settings for node/report/purge ttls

[nicklewis]

No description provided.

[cprice404]

I've been trying to put some thought into what our strategy is going to be for maintaining backward-compatibility with older versions of PuppetDB. Two of these settings are harmless on that front--though perhaps misleading. However, for the node-ttl setting we are not providing a way to define the older node-ttl-days setting here, which means we're exposing a setting that was available in 1.0 but not the means to actually set it.

I worry that the module is going to get really hairy if we try to maintain backward compatibility too far into the past, so I think my current inclination is to simply start documenting (very prominently--at the top of the main README or similar) that certain versions of the module are only compatible with certain versions of puppetdb. If you want to use an older version of puppetdb then you use an older version of the module.

Thoughts? @kbarber ?

[kbarber]

However, for the node-ttl setting we are not providing a way to define the older node-ttl-days setting here, which means we're exposing a setting that was available in 1.0 but not the means to actually set it.

If we can tell what version of the module they are using, its easy enough to make this conditional. If the semantics are the same the newer node-ttl option could set node-ttl-days or node-ttl conditionally. Question is, if we expose it and don't check - and yet we claim we support 1.0 in the module and someone uses it, what happens?

I worry that the module is going to get really hairy if we try to maintain backward compatibility too far into the past, so I think my current inclination is to simply start documenting (very prominently--at the top of the main README or similar) that certain versions of the module are only compatible with certain versions of puppetdb

@cprice-puppet I agree, we have to be clear on what version of puppetdb we support with the module. Its about being PuppetDB version focused ... not config option/feature focused.

Can we treat it like any feature and use semantic versioning and deprecation rules around this? Ie. 0.9 is now deprecated in say 2.0.5, next major release (3.0.0) its not supported any more. Keep it in line with how we support puppetdb itself would be useful, so if we are officially not releasing patches for 0.9 then the module gets deprecation messages and later unsupported?

[cprice404]

If someone uses the setting in the module but is running PuppetDB 1.0.x, I'm about 99% sure that it would just silently ignore the setting. I'm pretty darn sure that our config parsing does not currently check for extra/unsupported key/val pairs in the config files (though I would prefer if it did).

We could certainly use semantic versioning and deprecation rules, and that might be the best way to go... but it's going to cause a bunch of spaghetti code in here that makes me tempted to just document the compatibility constraints and not hassle with it. The former would probably be a better end user experience but I'm not 100% sure how much payoff there would be.

And yes, we'd have to add a fact or something that would detect what version of PuppetDB was installed (if any). Actually, I don't know if it could be a fact because then it wouldn't work on the first run (before they'd installed the package initially)... unless we had some if/pick statements that were smart enough to distinguish between the installed version (or lack thereof), the latest version available (if they're using 'present'), or the user-specified version (if they pass the param).

[kbarber]

Another angle on this ... @cprice @nicklewis ... So we can almost know what the version is today without a fact, by looking at the $puppetdb_version. However, if it is set to 'present' in the past, we are kind of stuck as it could be anything today. If its 'latest' we know its - well, the latest so whatever the 'latest' supported at the time of module release should work. This all seems a little dodgy though, as I'm guessing the majority will just not define a specific version and let if fall back to 'present'. So yeah, today - no good way to detect version.

Maybe we should just add the old parameter as an option. Ie. Add the old parameter node-ttl-days right ... saying in docs its only supported on 1.0.0 and is now deprecated if you are using 1.1.0. The new parameter is also provided, and we mention in docs it was introduced in 1.1.0. Effectively we are mapping the config to the class parameters 1 to 1 ... which is probably a reasonable approach, and it means that users familiar with our config docs can just use our class without having to do a mental mapping somewhere.

We don't have a super-great way of obviously doing proper validation on this without being certain of the puppetdb version, seems to me if we return a deprecation message in PuppetDB itself on node-ttl-days then at least we are warning somewhere. Do we do this today for example? If not we probably should have, or at least allow node-ttl-days to work as intended until a 2.0 release, based on semver.

[cprice404]

You're totally correct that puppetdb should be logging a deprecation warning on node-ttl-days, and it's not. D'oh.

Here's another random idea to throw out there... we could tie the semver of the module to the semver of puppetdb itself... So, we could do what you said for now (add support for node-ttl-days to 1.1 of the module) but not put the purge setting in there, and then we could release version 1.2 of the module and add the purge setting with it. Then from here on out we could simply document that you need to use the same semver series of the module as the version of puppetdb you're running. It'd be a slight bending of the semver rules for the module for this release but I think it would probably end up matching up fairly well going forward, and would prevent us from having to maintain any spaghetti code in the module for trying to guess which version you're running.

Seems like this would work pretty intuitively for the 90% use-case (where the user is installing the module and adding 'ensure=>present' to their manifests at the same time). It would mean that you'd want/need to upgrade the module when you upgrade puppetdb, but that seems fairly reasonable I think...

Thoughts?

[cprice404]

@NLEW : any opinion?

[nicklewis]

My concern about that approach is it prevents us from using semver for the module itself. If we want to add new module features (rather than support for new PuppetDB features), we have no way of conveying that in the version number in a semver-compatible way. We could just do a point release, but since the forge is pretty strict on semver, that seems bad.

There's another issue, which I discussed with @cprice-puppet in person just now, which is that we have to add a variable and ini_setting for every new param now, even though it's just key-value. Some settings, like the database-related ones, have their own logic so this makes sense. But for ordinary settings, maybe it would serve users better to add some simple define for puppetdb_setting or somesuch, which just wraps ini_setting and sets the filename. Then users don't need to wait for us to get around to releasing a new version of the module, and then upgrade, to be able to take advantage of new settings. It also reduces the duplication we face now, of having to add the parameter in three places and document it in there more. The main disadvantage of this is that users have to refer to the central PuppetDB documentation for the list of available parameters, rather than the PuppetDB module docs. Personally, I think that's not so bad.

@kbarber what are your thoughts on this?

[cprice404]

This approach seems reasonable to me too. Pro: module is more future-proof and we can use meaningful semver on it. Con: module documentation is not sufficient on its own to provide user with the info that they need to configure things... but as long as the module docs contain a link to the puppetdb docs I don't think that's too big of a deal.

[kbarber]

I was going to raise that concern earlier, I'm not a massive fan of the direct mapping from class/define parameter to config setting. Normally I have a defined resource for each setting - and have a class take a hash and use create_resources. This means you get class-time settings and later on users can add settings outside of the class if they desire.

Whatever the implementation is, like you say @nicklewis this means you can add features to puppetdb without having to have the module always keep up. 👍

[cprice404]

So we should close this pull, then, and open a new one that introduces a new defined resource type called puppetdb_setting, yes?

[kbarber]

Shouldn't we be exposing these new parameters for modification in puppetdb & puppetdb::server? I'm not 100% on this module, but just adding it to params doesn't expose it I believe.

Also - we'll need to update the README.md with any new parameters.

[cprice404]

Oh yeah--you're totally right, Ken, they need to be exposed in both of those two classes.

[nicklewis]

Amended the commit to add support for those params to puppetdb::server and puppetdb and to add documentation.

[kbarber]

Amended the commit to add support for those params to puppetdb::server and puppetdb and to add documentation.

Adding the docs to the .pp files is cool, but the fact is no one every sees this stuff @nicklewis ... no one runs puppet doc in the wild I found (unless they were a customer I set it up for), and we don't really want people opening the code if we can avoid it. *shrug. The params really need to be added to the README.md as it shows up on the forge then.

I really really really want a day when we grab this stuff from source and display it automatically on the forge, but we aren't there today. Sad thing is lots of us maintain it in two places. I'm starting to question this ... because I bet when the day comes forge can display this info, they'll change the format or something crazy :-).

(for some reason some of this comment is in italics, I'm too tired to care)

[nicklewis]

I was copying what was already there, which was both the .pp files and the README.md.

Eeeeeexcept apparently I somehow forgot to add them to the README. Go-go distracted development! Will do that now.

[nicklewis]

Alright, this time I really added them to the README!

[nicklewis]

@kbarber : I just discussed with @cprice-puppet, and we came to the conclusion that a puppetdb_setting type wouldn't actually add much value over the ini_setting which is already available. You would have to specify which section the setting goes in, and we could map that to filename, but that's about the only value you'd get out of it. It doesn't seem worth the effort or complexity compared to just using init_setting, which satisfies the same need of declaring arbitrary settings.

Does this seem reasonable to you? If so, do you think it's better to go ahead and merge this pull request adding specific settings and only use ini_setting as a fallback for those which haven't been added as "proper" settings, or to just require use of ini_settings as the general way of declaring settings and reject this pull request?

[kbarber]

Sure, the main reason I suggested a define resource is that if you took a config hash for config items as you were discussing that would be the nicest way to feed into create_resources to provide full future capability without users having to have deeper knowledge of how the ini_setting params are fed. The secondary effect is that users can have a more context aware interface as a defined resource.

However, for the sake of getting this particular feature done and done, I'm okay with someone just merging this - we can always prototype the alternative to see how it looks as a separate card/ticket. That way we don't bog down this change as its already dragging. I'll be happy to prototype this in Portland so we can discuss face to face, I don't imagine its that much work, very similar to the pg_hba_rule work I did for postgresql actually.

[cprice404]

OK so now we're back to the issue of those features not being supported on older versions of puppetdb. Are we OK with just documenting that and assuming it will be relatively harmless if someone tries to use them on an older version?

[nicklewis]

@cprice-puppet That's fine with me.

[cprice404]

@nicklewis ok so then do you want to update the pull to include some verbiage on that?

@kbarber where are you on the decision about when to do a new release of this module? If you are planning on doing that soon, then we should hold off on merging this until you do so. Otherwise I'll probably go ahead and merge as soon as @nicklewis adds docs.

[nicklewis]

Pushed a commit documenting the supported versions for those settings.

[cprice404]

Cool. @kbarber let us know about your release plans (if any); I'm ready to merge this otherwise.

[kbarber]

@cprice-puppet I think the release will be next week most probably at this point.

[cprice404]

OK, well then we should probably wait on merging this until after you do that, because I doubt we'll have released puppetdb 1.2 by then.

[puppetcla]

CLA Signed by nicklewis on 2010-08-23 21:00:00 -0700