Add pe facts to stdlib #99

haus · 2012-10-23T21:23:37Z

These commits add is_pe, pe_version, pe_major_version, pe_minor_version, and pe_patch_version to stdlib as well as some basic spec tests.

jeffmccune · 2012-10-23T21:25:38Z

lib/facter/pe_version.rb

+#
+Facter.add("pe_version") do
+  setcode do
+    pe_ver = Facter.value("puppetversion").match(/Puppet Enterprise (\d\.\d\.\d)/)


I think you mean \d+ in this regexp, no?

Ah yes, that is more correct and future proof.

Edit: Updated in b26d0f6

@jeffmccune Anything else jump out at you?

jeffmccune · 2012-10-24T19:54:08Z

Looking now.

jeffmccune · 2012-10-24T20:36:00Z

Some additional comments: b26d0f6 should be squashed into df46f0e. There should be test coverage for the multi-digit scenario like version 2.10.1.

Finally, why the call to Facter.clear and Facter.clear_messages?

jeffmccune · 2012-10-24T20:52:58Z

Some final comments on this one.

In the future, please use the branch naming convention of foo/<basebranch>/XXXXX_topic so that the common git aliases I use are able to parse out the base branch.

Second, the example descriptions for the negative case don't match the examples themselves. I've fixed this up in e4087d6 which should be squashed into e0349f0 as well.

Finally, an example of how to add coverage for multi-digit version numbers without duplicating a bunch of code is available at c834a59 in my topic branch. The improved descriptions are also nice to have, generic descriptions make it more difficult to understand the specification and the intended behavior.

Hope this helps. Once the topic branch is cleaned up I'm happy to merge this in.

My patches are published at: https://github.com/jeffmccune/puppetlabs-stdlib/tree/haus-add_pe_facts_to_stdlib

-Jeff

haus · 2012-10-24T22:34:37Z

the Facter.clear cleans up the Facts from the previous test so they don't pollute the current one. If you remove the after block, the second set of tests fail.

haus · 2012-10-24T22:37:16Z

The only thing about the updated tests that bothers me is that it's essentially duplicating the code path in the custom fact (they both call to split(".")).

It's not duplicating the path, it's just duplicating the behavior, right?

(I wrote the test without looking at the implementation, so I didn't realize I was duplicating the call to .split at the time. This is a "good thing" IMHO, since I was able to write a test about the behavior you described without knowing how it's implemented.

See what I mean by the difference between duplicating code and duplicating a code path?

jeffmccune · 2012-10-24T22:37:48Z

the Facter.clear cleans up the Facts from the previous test so they don't pollute the current one. If you remove the after block, the second set of tests fail.

Ah, cool. That smells to me; I expect the spec helper or something else to automatically handle cleaning up state between example groups. I'm going to dive in a bit to try and figure out why calling clear is necessary.

haus · 2012-10-24T22:40:35Z

Rebased and squashed back into 2 commits, including your fixes.

I removed Facter.clear_messages as it wasn't essential as far as i could tell.

haus · 2012-10-24T22:41:05Z

Yea, I think spec_helper in Facter does exactly that and is probably where I got the code from.

jeffmccune · 2012-10-24T22:44:53Z

The only thing about the updated tests that bothers me is that it's essentially duplicating the code path in the custom fact (they both call to split(".")).

Ah, so thinking about this a bit more... The litmus test I use when thinking about these sorts of problems is, "If the implementation changes, will this example notify me?"

I think in this situation (major,minor,patch) = version.split(".") is OK in the test because it doesn't depend on the implementation. If someone patches the fact in the future and it doesn't handle multiple digits then we'll catch the problem automatically, which is the whole point.

See what I'm driving at?

haus · 2012-10-24T22:49:37Z

For sure, it will definitely catch those problems.

jeffmccune · 2012-10-25T00:12:27Z

@haus

I figured it out. The patches that remove the need to call Facter.clear in all of the example groups are avialable in my branch. Please consider rebasing these patches into this topic branch.

I think we should be good to go once that's done.

5651ebe Fixup 220a7a3 Add PE facts to stdlib (improve readability) 13 seconds ago Jeff McCune (HEAD, jeffmccune/haus-add_pe_facts_to_stdlib, haus-add_pe_facts_to_stdlib)
b56230d Fixup 220a7a3 Add PE facts to stdlib 7 minutes ago Jeff McCune
c87ccad (maint) Clear all facts before each example 11 minutes ago Jeff McCune

As many PE modules have PE specific functionality, but are deployed to all nodes, including FOSS nodes, it is valuable to be able to selectively enable those PE specific functions. These facts allow modules to use the is_pe fact to determine whether the module should be used or not. The facts include is_pe, pe_version, pe_major_version, pe_minor_version, and pe_patch_version. For PE 2.6.0 those facts would have values true, 2.6.0, 2, 6, and 0, respectively.

This commit adds some basic spec tests for the pe_version facts. There are basic postitive and negative cases.

Without this patch example groups must explicitly call `Facter.clear` to clear any cached values between examples. This is a problem because this behavior is not the concern of the example groups, the behavior is the concern of the spec_helper or whatever facility we have in place to initialize the system for testing. This patch fixes the problem by duplicating the logic in the Facter spec_helper to ensure facts are cleared out before each example. This patch requires the example groups to explicitly load the facts they require if the fact name does not match the filename.

haus · 2012-10-25T00:26:06Z

@jeffmccune looks sane, rebased (thanks for figuring the spec helper thing out).

I tend to think not Facter.value(:pe_version).to_s.empty? is more readable than the if/else, but it's the same end.

jeffmccune · 2012-10-25T17:34:44Z

@haus

I merged this into 2.4.x and master:

$ git branch --contains cdf3b05
  2.4.x
* master

The last note I have is that I added e686779 on top of this change set. undefined methodsplit' for nil:NilClassis a pretty common error and Facts often resolve tonil` so I decided to add this small patch as a defensive measure.

Thanks for the contribution and thanks for taking the time and effort to organize the commits and provide a clear commit message. It really helped me work through this quickly.

-Jeff

jeffmccune · 2012-10-25T17:41:16Z

Actually... I just screwed this up.

I merged this new fact into 2.4.x but it's not fixing any bug. It's adding a new fact, so this should go into master and we should release 2.5 since this is new, backwards-compatbile functionality.

I'm going to revert this out of 2.4.x and merge it into master, then establish 2.5.x to cut the 2.5.0 release to the forge.

-Jeff

haus · 2012-10-25T17:46:28Z

@jeffmccune sounds great. thanks for working through it with me!

jeffmccune reviewed Oct 23, 2012
View reviewed changes

haus and others added 3 commits October 24, 2012 17:23

Add spec tests for pe_version facts

4442f1e

This commit adds some basic spec tests for the pe_version facts. There are basic postitive and negative cases.

jeffmccune merged commit ba70a38 into puppetlabs:2.4.x Oct 25, 2012

jordanbreen28 added the feature label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pe facts to stdlib #99

Add pe facts to stdlib #99

haus commented Oct 23, 2012

jeffmccune Oct 23, 2012

haus Oct 23, 2012

haus Oct 24, 2012

jeffmccune commented Oct 24, 2012

jeffmccune commented Oct 24, 2012

jeffmccune commented Oct 24, 2012

haus commented Oct 24, 2012

haus commented Oct 24, 2012

jeffmccune commented Oct 24, 2012

haus commented Oct 24, 2012

haus commented Oct 24, 2012

jeffmccune commented Oct 24, 2012

haus commented Oct 24, 2012

jeffmccune commented Oct 25, 2012

haus commented Oct 25, 2012

jeffmccune commented Oct 25, 2012

jeffmccune commented Oct 25, 2012

haus commented Oct 25, 2012

Add pe facts to stdlib #99

Add pe facts to stdlib #99

Conversation

haus commented Oct 23, 2012

jeffmccune Oct 23, 2012

Choose a reason for hiding this comment

haus Oct 23, 2012

Choose a reason for hiding this comment

haus Oct 24, 2012

Choose a reason for hiding this comment

jeffmccune commented Oct 24, 2012

jeffmccune commented Oct 24, 2012

jeffmccune commented Oct 24, 2012

haus commented Oct 24, 2012

haus commented Oct 24, 2012

jeffmccune commented Oct 24, 2012

haus commented Oct 24, 2012

haus commented Oct 24, 2012

jeffmccune commented Oct 24, 2012

haus commented Oct 24, 2012

jeffmccune commented Oct 25, 2012

haus commented Oct 25, 2012

jeffmccune commented Oct 25, 2012

jeffmccune commented Oct 25, 2012

haus commented Oct 25, 2012