cloud-init-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

On Tue, Oct 3, 2017 at 8:23 AM, Scott Moser <smoser@xxxxxxxxxx> wrote:

> i approve with 2 small questions in line.
>
>
> Diff comments:
>
> > diff --git a/cloudinit/user_data.py b/cloudinit/user_data.py
> > index 88cb7f8..de459c9 100644
> > --- a/cloudinit/user_data.py
> > +++ b/cloudinit/user_data.py
> > @@ -222,16 +223,25 @@ class UserDataProcessor(object):
> >              if include_once_on and os.path.isfile(include_once_fn):
> >                  content = util.load_file(include_once_fn)
> >              else:
> > -                resp = util.read_file_or_url(include_url,
> > -
>  ssl_details=self.ssl_details)
> > -                if include_once_on and resp.ok():
> > -                    util.write_file(include_once_fn, resp.contents,
> mode=0o600)
> > -                if resp.ok():
> > -                    content = resp.contents
> > -                else:
> > -                    LOG.warning(("Fetching from %s resulted in"
> > -                                 " a invalid http code of %s"),
> > -                                include_url, resp.code)
> > +                try:
> > +                    resp = util.read_file_or_url(include_url,
> > +
>  ssl_details=self.ssl_details)
> > +                    if include_once_on and resp.ok():
> > +                        util.write_file(include_once_fn, resp.contents,
> > +                                        mode=0o600)
> > +                    if resp.ok():
> > +                        content = resp.contents
> > +                    else:
> > +                        LOG.warning(("Fetching from %s resulted in"
> > +                                     " a invalid http code of %s"),
> > +                                    include_url, resp.code)
> > +                except UrlError as urle:
> > +                    LOG.warning(
> > +                        "Fetching from %s resulted in a UrlError: %s",
> > +                        include_url, urle.cause)
>
> i think just printing urle here in its string form should be reasonable.
> str(urle) would look like:
>  cloudinit.url_helper.UrlError('404 Client Error: Not Found for url:
> http://example.com/asdf')
> str(urle.cause) would look  like:
>  requests.exceptions.HTTPError('404 Client Error: Not Found for url:
> http://example.com/asdf')
>

> > +                except IOError as ioe:
> > +                    LOG.warning("Fetching from %s resulted in an
> IOError: %s",
> > +                                include_url, ioe.strerror)
>
> i guess fine, but is there a reason for 'ioe.strerror' rather than just
> str(ioe) ?
>
> >
> >              if content is not None:
> >                  new_msg = convert_string(content)
>
>
In both cases, using an attribute which is already a string will be faster
than calling str() which introduces some lookup and call overhead.

In an exception path, I don't think this matters; however generally I would
say we should avoid
additional function calls if they're not needed.

-- 
https://code.launchpad.net/~ajorgens/cloud-init/+git/cloud-init/+merge/331660
Your team cloud-init commiters is requested to review the proposed merge of ~ajorgens/cloud-init:_include-urlerror into cloud-init:master.