Open Stack

Okay, boiling my thoughts down further:

James's (valid, IMHO) concerns aside, I want to see one of two things
before I'm anything but -1 on this change:

1) A specific reason SHELLOPTS can't be used.  Nobody has given me one
besides hand-wavy "it might not work" stuff.  FTR, as I noted in my
previous message, the set -e thing can be easily addressed if we think
it necessary so I don't consider that a valid answer here.

Also, http://stackoverflow.com/questions/4325444/bash-recursive-xtrace

2) A specific use case that can only be addressed via this
implementation.  I don't personally have one, but if someone does then
I'd like to hear it.

I'm all for improving in this area, but before we make an intrusive
change with an ongoing cost that won't work with anything not explicitly
enabled for it, I want to make sure it's the right thing to do.  As yet
I'm not convinced.

-Ben

On 11/27/2014 12:29 PM, Sullivan, Jon Paul wrote:
>> -----Original Message-----
>> From: Ben Nemec [mailto:openstack at nemebean.com]
>> Sent: 26 November 2014 17:03
>> To: OpenStack Development Mailing List (not for usage questions)
>> Subject: Re: [openstack-dev] [diskimage-builder] Tracing levels for
>> scripts (119023)
>>
>> On 11/25/2014 10:58 PM, Ian Wienand wrote:
>>> Hi,
>>>
>>> My change [1] to enable a consistent tracing mechanism for the many
>>> scripts diskimage-builder runs during its build seems to have hit a
>>> stalemate.
>>>
>>> I hope we can agree that the current situation is not good.  When
>>> trying to develop with diskimage-builder, I find myself constantly
>>> going and fiddling with "set -x" in various scripts, requiring me
>>> re-running things needlessly as I try and trace what's happening.
>>> Conversley some scripts set -x all the time and give output when you
>>> don't want it.
>>>
>>> Now nodepool is using d-i-b more, it would be even nicer to have
>>> consistency in the tracing so relevant info is captured in the image
>>> build logs.
>>>
>>> The crux of the issue seems to be some disagreement between reviewers
>>> over having a single "trace everything" flag or a more fine-grained
>>> approach, as currently implemented after it was asked for in reviews.
>>>
>>> I must be honest, I feel a bit silly calling out essentially a
>>> four-line patch here.
>>
>> My objections are documented in the review, but basically boil down to
>> the fact that it's not a four line patch, it's a 500+ line patch that
>> does essentially the same thing as:
>>
>> set +e
>> set -x
>> export SHELLOPTS
> 
> I don't think this is true, as there are many more things in SHELLOPTS than just xtrace.  I think it is wrong to call the two approaches equivalent.
> 
>>
>> in disk-image-create.  You do lose set -e in disk-image-create itself on
>> debug runs because that's not something we can safely propagate,
>> although we could work around that by unsetting it before calling hooks.
>>  FWIW I've used this method locally and it worked fine.
> 
> So this does say that your alternative implementation has a difference from the proposed one.  And that the difference has a negative impact.
> 
>>
>> The only drawback is it doesn't allow the granularity of an if block in
>> every script, but I don't personally see that as a particularly useful
>> feature anyway.  I would like to hear from someone who requested that
>> functionality as to what their use case is and how they would define the
>> different debug levels before we merge an intrusive patch that would
>> need to be added to every single new script in dib or tripleo going
>> forward.
> 
> So currently we have boilerplate to be added to all new elements, and that boilerplate is:
> 
>     set -eux
>     set -o pipefail
> 
> This patch would change that boilerplate to:
> 
>     if [ ${DIB_DEBUG_TRACE:-0} -gt 0 ]; then
>         set -x
>     fi
>     set -eu
>     set -o pipefail
> 
> So it's adding 3 lines.  It doesn't seem onerous, especially as most people creating a new element will either copy an existing one or copy/paste the header anyway.
> 
> I think that giving control over what is effectively debug or non-debug output is a desirable feature.
> 
> We have a patch that implements that desirable feature.
> 
> I don't see a compelling technical reason to reject that patch.
> 
>>
>> -Ben
>>
>> _______________________________________________
>> OpenStack-dev mailing list
>> OpenStack-dev at lists.openstack.org
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> 
> 
> Thanks, 
> Jon-Paul Sullivan ☺ Cloud Services - @hpcloud
> 
> Postal Address: Hewlett-Packard Galway Limited, Ballybrit Business Park, Galway.
> Registered Office: Hewlett-Packard Galway Limited, 63-74 Sir John Rogerson's Quay, Dublin 2. 
> Registered Number: 361933
>  
> The contents of this message and any attachments to it are confidential and may be legally privileged. If you have received this message in error you should delete it from your system immediately and advise the sender.
> 
> To any recipient of this message within HP, unless otherwise stated, you should consider this message and attachments as "HP CONFIDENTIAL".
>