docker_image.py - UnboundLocalError

[rosscdh]

Description of Issue/Question

bad python reference

                File "/usr/lib/python3/dist-packages/salt/state.py", line 1878, in call
                  **cdata['kwargs'])
                File "/usr/lib/python3/dist-packages/salt/loader.py", line 1823, in wrapper
                  return f(*args, **kwargs)
                File "/usr/lib/python3/dist-packages/salt/states/docker_image.py", line 362, in present
                  if error:
              UnboundLocalError: local variable 'error' referenced before assignment

Setup

(Please provide relevant configs and/or SLS files (Be sure to remove sensitive info).)

std install

Steps to Reproduce Issue

(Include debug logs if possible and relevant.)

nada

Versions Report

(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)

sudo salt --versions-report
Salt Version:
           Salt: 2018.3.0

Dependency Versions:
           cffi: Not Installed
       cherrypy: 3.5.0
       dateutil: 2.4.2
      docker-py: Not Installed
          gitdb: 0.6.4
      gitpython: 1.0.1
          ioflo: Not Installed
         Jinja2: 2.8
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.6
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 3.5.2 (default, Nov 23 2017, 16:37:01)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 15.2.0
           RAET: Not Installed
          smmap: 0.9.0
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.1.4

System Versions:
           dist: Ubuntu 16.04 xenial
         locale: ANSI_X3.4-1968
        machine: x86_64
        release: 4.4.0-1054-aws
         system: Linux
        version: Ubuntu 16.04 xenial

[rosscdh]

#47487 PR for review

[gtmanfred]

Thanks for the PR for this.

Daniel

[rosscdh]

There is a similar problem in docker_container.py ill make a pr for it a bit later

[terminalmage]

@rosscdh thanks for the PR. I found no similar issue in docker_container.py, can you show me where you're seeing this?

[rosscdh]

curious, I stand corrected; must have been fog of war apologies for misdirection

[terminalmage]

@rosscdh No problem! Since you provided a PR to resolve this issue (thanks again for that!), I'll go ahead and close this.

[davelindquist-egistix]

If I read this PR correctly, it doesn't quite work as desired?

The problem code:

try:
    __salt__['docker.inspect_image'](full_image)
    error = False
except CommandExecutionError as exc:
    msg = exc.__str__()
    ...

The solution simply moves the error=False prior to the try -- but shouldn't the solution look more like:

try:
    __salt__['docker.inspect_image'](full_image)
    error = False
except CommandExecutionError as exc:
    error = True
    msg = exc.__str__()
    ...

to capture the intent?

[rosscdh]

Yes you could do it like that. I tend to prefer setting a default for a variable outside of any try catch clause which can then be overridden in a limited scope. as per the PR.. but the actual implementation is up to you :)

[davelindquist-egistix]

Right - the setting before or inside the try is just a matter of preference :-)

However, what I was getting at was the missing error = True -- if I read the PR correctly, error is guaranteed to always be False?

[gtmanfred]

It will always be False if docker.inspect_image does not throw a CommandExecutionError or other exception.

[davelindquist-egistix]

Maybe I'm missing something then...

The error that I see in the code (and that I encountered in practice, and that seems to be this issue) is that:

• The docker inspect image command fails
  • Therefore, the error=False that occurs after this doesn't get hit
• And there is no error=(anything) in the catch statement
  • Therefore error is unset at the following if

The fix (in the PR):

• moves the error=False to before the try, solving the 'unset variable' issue
• but there is STILL no error=(anything) in the catch statement
  • which means that error will never be True, even if there is an error?

Again, perhaps I'm missing something, but it seems like we need an error=True in the catch?