#18393 - diff args

[Alex Harvey]

Hi all,

I need to submit a patch to fix #18393 as it is affecting about half my fleet - turns out AIX<=5.3, Solaris<=8, and all HP-UX don't have a diff with -u.  I would like to submit a patch that allows us to specify diff args in the configuration file that override the defaults.  I'd just like to check in with everyone first that there are no in principle objections to what I'm doing.

Best regards,
Alex

[Dustin J. Mitchell]

The way 'diff' is hardcoded into the source is a little janky. It'd
be nice to allow that to be fully customized, rather than just
accepting args to add to a hard-coded 'diff'.

Dustin

[Andy Parker]

On Thu, May 2, 2013 at 6:59 PM, Dustin J. Mitchell <dus...@v.igoro.us> wrote:

On Thu, May 2, 2013 at 9:46 PM, Alex Harvey <alexh...@gmail.com> wrote:
> I need to submit a patch to fix #18393 as it is affecting about half my
> fleet - turns out AIX<=5.3, Solaris<=8, and all HP-UX don't have a diff with
> -u.  I would like to submit a patch that allows us to specify diff args in
> the configuration file that override the defaults.  I'd just like to check
> in with everyone first that there are no in principle objections to what I'm
> doing.

No principle objection

 

The way 'diff' is hardcoded into the source is a little janky.  It'd
be nice to allow that to be fully customized, rather than just
accepting args to add to a hard-coded 'diff'.

I do, however, agree with Dustin. The first time I found the diff call it was a bit of a WTF moment. I would be for either making the entire diff call configurable or replace the call to an external prog with a ruby diff implementation.

If we change the call (such as dropping -u in certain cases) could this cause any problems? Are there any systems that are using that diff output for anything? Or is this just a thing that humans read?

 

Dustin

--
You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to puppet-dev+...@googlegroups.com.
To post to this group, send email to puppe...@googlegroups.com.
Visit this group at http://groups.google.com/group/puppet-dev?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

--

Andrew Parker

an...@puppetlabs.com

Freenode: zaphod42

Twitter: @aparker42

Software Developer

Join us at PuppetConf 2013, August 22-23 in San Francisco - http://bit.ly/pupconf13
Register now and take advantage of the Early Bird discount - save 25%!

[Daniel Pittman]

On Fri, May 3, 2013 at 11:05 AM, Andy Parker <an...@puppetlabs.com> wrote:
> On Thu, May 2, 2013 at 6:59 PM, Dustin J. Mitchell <dus...@v.igoro.us>
> wrote:
>>
>> The way 'diff' is hardcoded into the source is a little janky. It'd
>> be nice to allow that to be fully customized, rather than just
>> accepting args to add to a hard-coded 'diff'.
>>
>
> I do, however, agree with Dustin. The first time I found the diff call it
> was a bit of a WTF moment. I would be for either making the entire diff call
> configurable or replace the call to an external prog with a ruby diff
> implementation.
>
> If we change the call (such as dropping -u in certain cases) could this
> cause any problems? Are there any systems that are using that diff output
> for anything? Or is this just a thing that humans read?

They are only for human consumption. Dropping the `-u` where
supported, though, is a huge loss for humans - the classic "ed" diffs
are not exactly friendly to read.

--
Daniel Pittman
⎋ Puppet Labs Developer – http://puppetlabs.com
♲ Made with 100 percent post-consumer electrons

[Nan Liu]

On Fri, May 3, 2013 at 11:05 AM, Andy Parker <an...@puppetlabs.com> wrote:

On Thu, May 2, 2013 at 6:59 PM, Dustin J. Mitchell <dus...@v.igoro.us> wrote:

On Thu, May 2, 2013 at 9:46 PM, Alex Harvey <alexh...@gmail.com> wrote:
> I need to submit a patch to fix #18393 as it is affecting about half my
> fleet - turns out AIX<=5.3, Solaris<=8, and all HP-UX don't have a diff with
> -u.  I would like to submit a patch that allows us to specify diff args in
> the configuration file that override the defaults.  I'd just like to check
> in with everyone first that there are no in principle objections to what I'm
> doing.

No principle objection

 

The way 'diff' is hardcoded into the source is a little janky.  It'd
be nice to allow that to be fully customized, rather than just
accepting args to add to a hard-coded 'diff'.

I do, however, agree with Dustin. The first time I found the diff call it was a bit of a WTF moment. I would be for either making the entire diff call configurable or replace the call to an external prog with a ruby diff implementation.

If we change the call (such as dropping -u in certain cases) could this cause any problems? Are there any systems that are using that diff output for anything? Or is this just a thing that humans read?

I can understand if it's a request to change the default setting, but isn't this already configurable? for both the diff tool and diff args?

[main]

  diff=''

  diff_args = ''

$ puppet apply ~/tmp/file.pp --show_diff --diff='ls' --diff_arg='-l'

notice: /Stage[main]//File[/tmp/a]/content:

-rw-r--r--  1 nan  wheel  9 May  3 15:09 /tmp/a

-rw-------  1 nan  staff  5 May  3 15:09 /var/folders/6j/mpcsjgt17n30_lc8bk9_b93r0000gn/T/puppet-file20130503-55060-mngkym

Nan

[Dustin J. Mitchell]

Nan, you're right - I remember looking at this a while ago, so perhaps
it was in some old version.

Dustin

[Alex Harvey]

On Saturday, May 4, 2013 6:20:11 AM UTC+10, Dustin J. Mitchell wrote:

Nan, you're right - I remember looking at this a while ago, so perhaps
it was in some old version.

Yes, agreed - and documented here, http://docs.puppetlabs.com/references/latest/configuration.html#diffargs  Tested it and it works on my fleet, so this is a good outcome for me. :-)  I'll close the Redmine ticket.

[Andy Parker]

It looks like the PE team hit this problem for AIX and created a fix
where they change the default diff_args based on the platform:
https://github.com/puppetlabs/puppet/pull/1637

So it looks like puppet is going to try and be smart by default.

[Alex Harvey]

On Wednesday, May 15, 2013 4:54:33 AM UTC+10, Andy Parker wrote:

It looks like the PE team hit this problem for AIX and created a fix
where they change the default diff_args based on the platform:
https://github.com/puppetlabs/puppet/pull/1637

So it looks like puppet is going to try and be smart by default.

I have two objections to that solution.

Firstly, AIX5.3 isn't the only operating system not supporting diff -u - it's a problem on all HP-UX, as well as on some legacy Solaris.

Secondly, if diff -u isn't available it may make sense to try diff -c instead.  The output from that is I think more readable.

If we were making Puppet smarter, perhaps the thing to do is make an additional call to 'diff' to see if it supports the default args - if it doesn't it will return exit code 2.

I suppose I could submit a patch that does this if the solution sounds good.  That said, I think being able to configure the diff_args in puppet.conf is also an adequate solution.

[Andy Parker]

On Tue, May 14, 2013 at 9:30 PM, Alex Harvey <alexh...@gmail.com> wrote:
>
>
> On Wednesday, May 15, 2013 4:54:33 AM UTC+10, Andy Parker wrote:
>>
>> It looks like the PE team hit this problem for AIX and created a fix
>> where they change the default diff_args based on the platform:
>> https://github.com/puppetlabs/puppet/pull/1637
>>
>> So it looks like puppet is going to try and be smart by default.
>
>
> I have two objections to that solution.
>
> Firstly, AIX5.3 isn't the only operating system not supporting diff -u -
> it's a problem on all HP-UX, as well as on some legacy Solaris.
>

Right, I hadn't noticed at first that AIX was not part of the original
report. I had read the issue after it had been updated to say AIX and
had missed the HP-UX part.

> Secondly, if diff -u isn't available it may make sense to try diff -c
> instead. The output from that is I think more readable.
>
> If we were making Puppet smarter, perhaps the thing to do is make an
> additional call to 'diff' to see if it supports the default args - if it
> doesn't it will return exit code 2.
>

My problem with all of these solutions is that they all increase the
startup time for everything. The one that was submitted calls facter
on startup, and your solution would execute diff (and presents other
issues related to getting what the diff program is since that is
configurable). That doesn't seem ideal, and granted, it also isn't
ideal that puppet can't run with the default settings on some
platforms.

> I suppose I could submit a patch that does this if the solution sounds good.
> That said, I think being able to configure the diff_args in puppet.conf is
> also an adequate solution.
>

I would be interested in seeing the proposal, but I almost get the
sense that the installation process should really just be laying down
the right default puppet.conf.

[Chris Price]

Andy,

Sorry, for some reason when I was reviewing that pull request I was thinking it was targeted at our AIX branch. I probably should have checked with your team before merging it.  If you guys decide that we should back that out and solve the problem a different way, we will be happy to work with you on that.

[Jeff Weiss]

On Tue, May 14, 2013 at 9:30 PM, Alex Harvey <alexh...@gmail.com> wrote:

That said, I think being able to configure the diff_args in puppet.conf is also an adequate solution.

As you noted diffargs is already configurable in puppet.conf. 

The pull request that Andy mentioned and that Chris merged is to provide an adequate default diffargs on all of our commercially supported platforms. Diff is off by default. If one goes into puppet.conf to enable diff, then on our commercially support platforms it will just work. On anything outside our commercially support platforms (as is the case for HP-UX and legacy Solaris), one is in puppet.conf anyway to turn on diff and can therefore set diffargs to something reasonable on the unsupported platform.

[Alex Harvey]

On Thursday, May 16, 2013 3:31:09 AM UTC+10, Jeff Weiss wrote:

The pull request that Andy mentioned and that Chris merged is to provide an adequate default diffargs on all of our commercially supported platforms. Diff is off by default. If one goes into puppet.conf to enable diff, then on our commercially support platforms it will just work. On anything outside our commercially support platforms (as is the case for HP-UX and legacy Solaris), one is in puppet.conf anyway to turn on diff and can therefore set diffargs to something reasonable on the unsupported platform.

Fair enough, but if you're going to keep this approach in the code and willing to pay the price of calling facter, then it would be trivial to have it just work the same way on all platforms - after all you may end up with a paying customer who just happens to have some old AIX5.2 lying around or legacy Solaris.