(MODULES-1738) Don't modify the global seed in fqdn_rotate()#406
Merged
cmurphy merged 1 commit intopuppetlabs:masterfrom Feb 12, 2015
Merged
Conversation
ffb46ee to
0c71e88
Compare
9e79d98 to
2e7efda
Compare
Contributor
Author
There was a problem hiding this comment.
In the event that we change fqdn_rotate() so that it uses fqdn_rand() to generate numbers in order to avoid duplicating Puppet::Util.deterministic_rand, we'll need to modify this test to only execute on Puppet >=3.2.0. One way to do that would be to extract the if Puppet::Util.respond_to?(:deterministic_rand) from the preceding test and have it wrap both the preceding test and this one.
As per puppetlabs/puppet@292233c, this leaves the global seed in a deterministic state, which is bad. Puppet::Util.deterministic_rand() exists to avoid running into this issue, but is only present starting in Puppet 3.2.0.
2e7efda to
84f866f
Compare
Contributor
Author
|
Rebased onto master, so tests should pass soon. |
Contributor
|
I think this implementation is fine, and it makes it easy to remove the reimplemented code when it's appropriate. Thanks! |
cmurphy
added a commit
that referenced
this pull request
Feb 12, 2015
…_seed (MODULES-1738) Don't modify the global seed in fqdn_rotate()
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
MODULES-1738 is similar to Redmine-15791, which was addressed in puppetlabs/puppet@292233c. The current implementation of fqdn_rotate() leaves the global seed in a deterministic state, which is bad. Puppet::Util.deterministic_rand() exists to avoid running into this issue, but is only present starting in Puppet 3.2.0.
An alternative (and, IMO, superior) way to handle this would be to instead replace lines 34-37 of fqdn_rotate.rb with:
This would have the benefits of being much cleaner and not having duplicated code from the Puppet codebase that would have to be maintained if for some reason it changed there. On the other hand, it would not prevent fqdn_rotate() from poisoning the global seed in versions of Puppet prior to 3.2.0. That being said, since fqdn_rand() already poisons the global seed in versions of Puppet prior to 3.2.0, maybe it doesn't matter if fqdn_rotate() does the same in those versions.