New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update module.py #29467

Merged
merged 1 commit into from Dec 7, 2015

Conversation

Projects
None yet
4 participants
@serge-p
Contributor

serge-p commented Dec 6, 2015

using module.run to check if port is opened on my host,
using module network.connect

example state:

network.connect:
module.run:
- host: {{ salt'network.ipaddrs'|join }}
- port: 1234
- proto: udp

found unexpected behavior:
module.run will always success as a result
even If network.connect returns result: False

example output (before the change):

      ID: network.connect
Function: module.run
  Result: True
 Comment: Module function network.connect executed
 Started: 22:28:29.475623
Duration: 0.001 ms
 Changes:   
          ----------
          ret:
              ----------
              comment:
                  Unable to connect to 10.0.0.50 (10.0.0.50) on tcp port 1234
              result:
                  False

Proposal:

Proposing to check
if changes_ret.result exists, and if it's boolean return it as a result
before attempting to check changes_ret.retcode

tested expected behavior:

      ID: network.connect
Function: module.run
  Result: False
 Comment: Module function network.connect executed
 Started: 22:50:25.149832
Duration: 9.964 ms
 Changes:   
          ----------
          ret:
              ----------
              comment:
                  Unable to connect to 10.0.0.50 (10.0.0.50) on tcp port 1234
              result:
                  False

      ID: network.connect
Function: module.run
  Result: True
 Comment: Module function network.connect executed
 Started: 22:50:07.129629
Duration: 0.205 ms
 Changes:   
          ----------
          ret:
              ----------
              comment:
                  Successfully connected to 10.0.0.50 (10.0.0.50) on tcp port 53
              result:
                  True
Update module.py
using module.run to check if port 53 is opened on localhost, using module network.connect  
example state: 
~~~
network.connect:
  module.run: 
    - host: {{ salt['network.ipaddrs']()|join }}
    - port: 53
    - port: 1234
    - proto: udp

unexpected behavior: 
module.run will always succeed, 
even If network.connect returns result: False

example output (before the change): 
----------
          ID: network.connect
    Function: module.run
      Result: True
     Comment: Module function network.connect executed
     Started: 22:28:29.475623
    Duration: 0.001 ms
     Changes:   
              ----------
              ret:
                  ----------
                  comment:
                      Unable to connect to 10.0.0.50 (10.0.0.50) on tcp port 1234
                  result:
                      False

Proposal: 
Proposing to check changes_ret.result and return it as a result before trying to check changes_ret.retcode 

tested expected behavior: 
~~~
----------
          ID: network.connect
    Function: module.run
      Result: False
     Comment: Module function network.connect executed
     Started: 22:50:25.149832
    Duration: 9.964 ms
     Changes:   
              ----------
              ret:
                  ----------
                  comment:
                      Unable to connect to 10.0.0.50 (10.0.0.50) on tcp port 1234
                  result:
                      False

----------
          ID: network.connect
    Function: module.run
      Result: True
     Comment: Module function network.connect executed
     Started: 22:50:07.129629
    Duration: 0.205 ms
     Changes:   
              ----------
              ret:
                  ----------
                  comment:
                      Successfully connected to 10.0.0.50 (10.0.0.50) on tcp port 53
                  result:
                      True

cachedout added a commit that referenced this pull request Dec 7, 2015

@cachedout cachedout merged commit 4e9aa1f into saltstack:develop Dec 7, 2015

4 of 6 checks passed

default Merged build finished.
Details
jenkins/salt-pr-rs-cent6-n Salt PR - RS CentOS 6 #509 — FAILURE
Details
jenkins/salt-pr-clone Salt PR - Clone Repository #11802 — SUCCESS
Details
jenkins/salt-pr-linode-ubuntu14.04-n Salt PR - Linode Ubuntu 14.04 #2866 — SUCCESS
Details
jenkins/salt-pr-lint-n Salt PR - Code Lint #11507 — SUCCESS
Details
jenkins/salt-pr-rs-cent7-n Salt PR - RS CentOS 7 #10335 — SUCCESS
Details
@cachedout

This comment has been minimized.

Show comment
Hide comment
@cachedout

cachedout Dec 7, 2015

Contributor

This is a great catch. Thank you very much @serge-p

Contributor

cachedout commented Dec 7, 2015

This is a great catch. Thank you very much @serge-p

cachedout added a commit that referenced this pull request Dec 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment