Skip to content

Rework apt::key to use apt_key. #230

Merged
merged 2 commits into from Mar 5, 2014

3 participants

@daenney
Puppet member
daenney commented Feb 20, 2014

Introducing a totally rewritten and tested apt::key. This commit also patches the spec's of apt::source because it was passing in data that is no longer allowed by the new validation rules in apt::key.

It does its best to not touch any other specs and where we touch them only minimally to ensure that we're not introducing breaking changes.

@daenney
Puppet member
daenney commented Feb 20, 2014

There are currently two spec's failing that I can't figure out how to fix. It stems from https://github.com/puppetlabs/puppetlabs-apt/blob/master/spec/defines/source_spec.rb#L138.

@apenney @hunner Could you pull and see if we can easily fix this?

@apenney
apenney commented Feb 20, 2014

You can just change the nils in the default_params hash to false. I mean, you can't pass a nil into a parameter anyway. The tests are a little clumsy in that they are looking for apt_key{} to contain things that may not actually be passed into apt_key in the first place.

Either way, defaulting them to false will get the tests to pass, how much further you want to go from there tearing apart the hashes and being more careful about only checking for things that you KNOW are in the param_hash (which could be done by wrapping those if checks in if :param_hash['x'] { it { checks for x }}, well, I'll leave that decision to you.

@daenney
Puppet member
daenney commented Feb 20, 2014

I don't think I can change them to false. Apt::key validates the input and if passed in a boolean isn't a valid value.

@apenney
apenney commented Feb 20, 2014

I dunno about that, I changed them to false and ran the tests before writing this comment and they passed.. so ;)

@daenney
Puppet member
daenney commented Feb 20, 2014

@apenney Damn, you're right, no idea why I changed that then.

I want to touch the rest of the test suite as little as possible so that we can merge this in knowing we didn't break existing behaviour. After that I think the test suite is long overdue for a refactor.

@apenney apenney and 1 other commented on an outdated diff Feb 20, 2014
.fixtures.yml
@@ -2,6 +2,6 @@ fixtures:
repositories:
"stdlib":
"repo": "git://github.com/puppetlabs/puppetlabs-stdlib.git"
- "ref": "v2.2.1"
+ "ref": "3.2.0"
@apenney
apenney added a note Feb 20, 2014

This may cause us troubles. Apt ships with PE, and PE uses stdlib 2.2. It's kind of an awkward situation because we're trying to tread a middle ground between not making it harder for PE users until a permanent solution is created to deal with this problem in PE.

As a kind of workaround for postgres we ended up making pe_postgres in PE, which the same public module with all the resources and classes renamed to add pe_, which isn't cool.

Basically what I'm saying here is we can bump it if we needed functionality in 3.2, but otherwise we should leave it at 2.2 until PE 3.3 or so when we'll hopefully have a permanent solution to the self inflicted pain we've caused everyone. :D

@daenney
Puppet member
daenney added a note Feb 20, 2014

I'll revert it. It was just annoying me that validate_re in 2.2.x doesn't support a third argument that allows me to pass in an error message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hunner
Puppet member
hunner commented Feb 20, 2014

What about stdlib 3.2.0? Since we're doing a major release, I think we could bump to that one.

@daenney
Puppet member
daenney commented Feb 20, 2014

@hunner: See @apenney's comment on the outdated diff. Since PE 3.2 still ships with 2.2.1 and PE 3.3 won't be there yet... I was under the impression people could update stdlib in PE but apparently that's not 'supported'.

@hunner
Puppet member
hunner commented Feb 20, 2014

@daenney But you're targeting master, which is already geared up for a 2.0.0 release. So we can change dependencies (unless you plan on backporting this to 1.4.x as a bugfix).

@daenney
Puppet member
daenney commented Feb 20, 2014

I'm all for raising the dependency, it's just that @apenney seemed to think we shouldn't do that.

@hunner
Puppet member
hunner commented Feb 21, 2014

Arg. I don't want to delay apt 2.0.0 until PE 3.3 comes out, but then I don't want to have to require PE users to upgrade stdlib (which I don't think will hurt anything, but yeah it makes our support team's job harder).

And we can't really bump the dependency without a major release.

@daenney
Puppet member
daenney commented Feb 21, 2014

I'll let you fight this out / coordinate this internally. For now I'll leave stdlib to 2.2.1 in this PR. Should the outcome be that we can bump to 3.2.0 before a 2.0.0 release I can always send a PR with the changes.

@daenney
Puppet member
daenney commented Feb 21, 2014

This is proving to be more difficult than I thought because of the initial 'dubious' choices to make a lot of parameters default to false instead of undef.

Right now the Beaker tests fail because apt::source by default passes in key, key_content, and key_source as false to apt::key which doesn't pass the validation rules.

If I change apt::source to have defaults of undef, this is what they actually should be imho, for key, key_content and key_source a bunch of rspec-puppet tests start failing because apt::source does some explicit testing on wether the key, key_content or key_source are false instead of falsey which causes the key to never be declared and hence not found in the catalog.

I'm inclined to fix apt::source to behave correctly. Looking at the source the changes would be minimal to both source.pp and the associated specs.

Anyone have a reason why I shouldn't?

@daenney
Puppet member
daenney commented Feb 21, 2014

I just pushed the changes I suggested. As you can see it's minimal and I haven't had to touch any of the acceptance tests, those pass with flying colours (just green though).

So, thoughts, comments etc! If everyone's okay with it I'll squash it and update the PR as ready.

@daenney
Puppet member
daenney commented Feb 22, 2014

@apenney @hunner: How do I write an acceptance test for 'key_source with a Puppet URI'?

@daenney
Puppet member
daenney commented Feb 26, 2014

Too many things were getting intertwined in this commit. I've chucked out a bunch of things I had in different commits. It's now just the apt::key changes as well as the really minor things in apt::source I had to change.

I've purposefully not touched the acceptance tests for apt::key so that we can verify that at merge this still behaved as before, I'll get to expanding test coverage and fixing #213 in subsequent PR's.

@apenney @hunner: Merge please?

Daniele Slui... added some commits Feb 20, 2014
Daniele Sluijters apt:🔑 Redo apt::key, make it use apt_key.
Introducing a totally rewritten and tested apt::key. This commit also
patches the spec's of apt::source because it was passing in data that
is no longer allowed by the new validation rules in apt::key.

It does its best to not touch any other specs and where we touch them
only minimally to ensure that we're not introducing breaking changes.
5790341
Daniele Sluijters Update the README about apt_key and apt::key. b2ae1cb
@apenney apenney merged commit ab6f6d3 into puppetlabs:master Mar 5, 2014

1 check passed

Details default The Travis CI build passed
@daenney daenney deleted the unknown repository branch Mar 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.