Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Configurable digest algorithm 14aug2012 #1035

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

jaredjennings commented Aug 14, 2012

Let the admin configure in puppet.conf which digest algorithm to use for file resources. I think these changes fulfill https://projects.puppetlabs.com/issues/8120.

These are the same changes as the original configurable digest algorithm pull request, #195, but rebased on a more recent master-branch commit.

The ill-fated branch configurable-digest-algorithm-aug2012, and its pull request #1033, are retracted in favor of this branch and pull request.

Contributor

hlindberg commented Oct 4, 2012

Hi, I just started looking at your pull request. There are a couple of hoops that you need to jump through. Check out the document https://github.com/puppetlabs/puppet/blob/master/CONTRIBUTING.md regarding accepting the contributor agreement (assuming https://projects.puppetlabs.com/users/2755 this is you on redmine). Please also update the commit messages to conform to the standard way of writing them as described in the same document . Your commit messages are almost in the correct format.

Contributor

hlindberg commented Oct 4, 2012

ok, so there are bigger things lurking regarding backwards compatibility... and the commits have already been reviewed

It looks like this is still defaulting to md5 instead of falling back to it?

Contributor

jeffmccune commented Apr 15, 2013

On Mon, Apr 15, 2013 at 9:11 AM, pwolanin notifications@github.com wrote:

It looks like this is still defaulting to md5 instead of falling back to
it?

I'm not sure I understand what you mean. Could you explain a bit more? Is
this configurable digest not working as expected? If so, what's the actual
behavior as compared to the expected behavior?

Thanks,
-Jeff

@jaredjennings jaredjennings commented on the diff Apr 16, 2013

lib/puppet/defaults.rb
@@ -565,6 +565,11 @@ module Puppet
:type => :boolean,
:desc => "Whether certificate revocation should be supported by downloading a Certificate Revocation List (CRL)
to all clients. If enabled, CA chaining will almost definitely not work.",
+ },
+ :digest_algorithm => {
+ :default => 'md5',
@jaredjennings

jaredjennings Apr 16, 2013

Contributor

@jeffmccune @pwolanin I believe this line encodes the behavior in question. It's my only concession so far to backward compatibility: if this PR were pulled tomorrow, it wouldn't break anyone's Puppet installation until they changed the puppet.conf to use a different digest_algorithm.

@pwolanin

pwolanin Apr 16, 2013

Yes, that's what I was looking at - what happens to an existing install if this value is changed?

I don't know what version of puppet this is targeted for - obviously in a stable erelease series a conservative default might make sense, but I'd how to see it using sha256 by default in the next major release at least.

@jaredjennings

jaredjennings Apr 16, 2013

Contributor

What happens to an existing install, indeed? That's why this code hasn't been merged already. Likely, what happens is that Puppet, or you, may search for files using the new digest algorithm that were bucketed using the old digest algorithm, and not find them. What happens then, I'm not really sure: I didn't really learn how file bucketing works before doing this patch. All that anyone from Puppet Labs has said is that it's a definite migration concern. I was hoping someone else would know better than me how to fix that concern but no one has stepped up. (No hard feelings meant.)

This particular branch was made from the puppetlabs/puppet master branch about a month before 3.0 was released. I haven't moved to 3.x yet, so here it sits.

Waiting for CLA signature by @jaredjennings

@jaredjennings - We require a Contributor License Agreement (CLA) for people who contribute to Puppet, but we have an easy click-through license with instructions, which is available at https://cla.puppetlabs.com/

Note: if your contribution is trivial and you think it may be exempt from the CLA, please post a short reply to this comment with details. http://docs.puppetlabs.com/community/trivial_patch_exemption.html

CLA signed by all contributors.

Member

adrienthebo commented Sep 16, 2013

@jaredjennings sincere apologies about the delay in response on this pull request! We've been playing catch up with pull requests that slipped through the cracks and I just got to this one.

This pull request is targeted at master but my understanding is that it was originally written for 2.7.x and would need to be updated to the current version of master to be applied. In addition, I did a quick review of this pull request, and it looks like that this added a global :digest_algorithm setting but also added new digest algorithms, which makes it a bit more cumbersome to review this pull request. If we split out some of the non-essential changes into new pull requests, it would mean that we would be able to incrementally merge changes instead of an all or nothing sort of deal.

At this point, where would you like to see this pull request go? Would you like to work with us to get this updated for master? Would it be easier to start with a new pull request? I'm mainly hoping to get some forward action for this, so that we have one less neglected pull request.

Contributor

jaredjennings commented Sep 16, 2013

Adrien, I've been meaning to re-do the pull request for a while. I'm delighted to hear a way for it to be easier to... ah, digest ;) and I'd love more attention on it, both mine and yours.

Adrien Thebo notifications@github.com wrote:

@jaredjennings sincere apologies about the delay in response on this
pull request! We've been playing catch up with pull requests that
slipped through the cracks and I just got to this one.

This pull request is targeted at master but my understanding is that it
was originally written for 2.7.x and would need to be updated to the
current version of master to be applied. In addition, I did a quick
review of this pull request, and it looks like that this added a global
:digest_algorithm setting but also added new digest algorithms, which
makes it a bit more cumbersome to review this pull request. If we split
out some of the non-essential changes into new pull requests, it would
mean that we would be able to incrementally merge changes instead of an
all or nothing sort of deal.

At this point, where would you like to see this pull request go? Would
you like to work with us to get this updated for master? Would it be
easier to start with a new pull request? I'm mainly hoping to get some
forward action for this, so that we have one less neglected pull
request.


Reply to this email directly or view it on GitHub:
#1035 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

Member

adrienthebo commented Sep 16, 2013

I would be more than happy to help out on this, and you can contact myself and the other Puppet developers via irc.freenode.net in #puppet-dev. We're usually available around 9 - 5 GMT -7, so if you just ask for help then I or one of the other developers should be able to help out. (For reference, my irc nick is finch).

In the mean time would you like to continue with this pull request or would you like to open a new one?

Contributor

jaredjennings commented Sep 16, 2013

I think this pull request should be closed and one or more new pull requests made as you suggested. (I'd do it myself, but I'm not in the same place as my Github and issues.puppetlabs.com passwords. Oops.)

Adrien Thebo notifications@github.com wrote:

I would be more than happy to help out on this, and you can contact
myself and the other Puppet developers via irc.freenode.net in
#puppet-dev. We're usually available around 9 - 5 GMT -7, so if you
just ask for help then I or one of the other developers should be able
to help out. (For reference, my irc nick is finch).

In the mean time would you like to continue with this pull request or
would you like to open a new one?


Reply to this email directly or view it on GitHub:
#1035 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

Member

adrienthebo commented Sep 16, 2013

In that case, I'm going to close this and we can start on this in a new pull request. Thanks for your understanding and patience on this!

@jaredjennings jaredjennings deleted the jaredjennings:configurable-digest-algorithm-14aug2012 branch Mar 19, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment