Salt: 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]

https://github.com/saltstack/salt/pull/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?

[gtmanfred]

Sorry, i was looking at what you had posted first, and thought that is what the pr was changed too, yes you are correct, what you listed is what this should be changed too, otherwise error is always set to False. and never set to True.

[davelindquist-egistix]

OK, no worries! Thanks!

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[terminalmage]

This was corrected long ago, the error variable is now defined above the try/except. Please close this.

[stale[bot]]

Thank you for updating this issue. It is no longer marked as stale.