Skip to content
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

prereq doesn't detect the prereq'd state's changes when using test=True #22024

Closed
boboli opened this issue Mar 26, 2015 · 5 comments
Closed

prereq doesn't detect the prereq'd state's changes when using test=True #22024

boboli opened this issue Mar 26, 2015 · 5 comments
Labels
Bug broken, incorrect, or confusing behavior Core relates to code central or existential to Salt P4 Priority 4 severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around stale
Milestone

Comments

@boboli
Copy link

boboli commented Mar 26, 2015

Easier to explain with examples:

asdf.sls:

prereq_command:
    cmd.run:
        - name: echo prereq
        - prereq:
            - cmd: asdf

asdf:
    cmd.wait:
        - name: echo asdf
        - watch:
            - cmd: fdsa

fdsa:
    cmd.run:
        - name: echo fdsa

I expect all 3 states to be run in the order of: fdsa, prereq_command, asdf

test=True output:

local:
----------
          ID: fdsa
    Function: cmd.run
        Name: echo fdsa
      Result: None
     Comment: Command "echo fdsa" would have been executed
     Started: 22:06:51.962681
    Duration: 1.887 ms
     Changes:
----------
          ID: prereq_command
    Function: cmd.run
        Name: echo prereq
      Result: True
     Comment: No changes detected
     Started:
    Duration:
     Changes:
----------
          ID: asdf
    Function: cmd.wait
        Name: echo asdf
      Result: True
     Comment:
     Started: 22:06:51.966291
    Duration: 1.763 ms
     Changes:

Summary
------------
Succeeded: 3 (unchanged=1)
Failed:    0
------------
Total states run:     3

The test=True output claims that prereq_command won't be run, which is incorrect. Interesting enough, the state that is prereq'd (asdf) shows that it will be run but is shown in green not bold yellow.

When I run the state normally:

actual output (no test=True):

local:
----------
          ID: fdsa
    Function: cmd.run
        Name: echo fdsa
      Result: True
     Comment: Command "echo fdsa" run
     Started: 22:13:16.788309
    Duration: 46.629 ms
     Changes:
              ----------
              pid:
                  29771
              retcode:
                  0
              stderr:

              stdout:
                  fdsa
----------
          ID: prereq_command
    Function: cmd.run
        Name: echo prereq
      Result: True
     Comment: Command "echo prereq" run
     Started: 22:13:16.836022
    Duration: 45.985 ms
     Changes:
              ----------
              pid:
                  29772
              retcode:
                  0
              stderr:

              stdout:
                  prereq
----------
          ID: asdf
    Function: cmd.wait
        Name: echo asdf
      Result: True
     Comment: Command "echo asdf" run
     Started: 22:13:16.884032
    Duration: 45.527 ms
     Changes:
              ----------
              pid:
                  29773
              retcode:
                  0
              stderr:

              stdout:
                  asdf

Summary
------------
Succeeded: 3 (changed=3)
Failed:    0
------------
Total states run:     3

This exhibits the correct and expected behavior, however test=True mode didn't seem to agree. Reading the documentation on prereq, I believe having test=True mode be truthful is critical to the correctness of prereq states.

@rallytime
Copy link
Contributor

@boboli I think you're right - there looks to be a bug here. I suspect we just didn't think through this case where prereq is interacting with watch while also running a test=True. The changes output should say something like "changes were detected and this state is set to run".

@rallytime rallytime added Bug broken, incorrect, or confusing behavior severity-low 4th level, cosemtic problems, work around exists labels Mar 26, 2015
@rallytime rallytime added this to the Approved milestone Mar 26, 2015
@boboli
Copy link
Author

boboli commented Mar 26, 2015

Hm, @basepi seemed to have different views on whether prereq should interact nicely with watched statements (see my other issue #22026). Or maybe I'm misunderstanding something here? I guess cmd.wait doesn't even make sense to run unless it's triggered by a watched state.

@rallytime
Copy link
Contributor

I've actually chatted with @basepi about both of your issues, and I think that while these are similar bugs, this one demonstrates a better "proof of concept" bug where the other one could have a likely work around. This one seems like a more general "we didn't think this interaction through and now we're hitting the bug" while the other one, while exhibiting the same behavior, has a useful workaround.

Though @basepi might disagree here and I have misunderstood him.

@basepi
Copy link
Contributor

basepi commented Mar 26, 2015

Hehe, depends on your definition of "interact nicely". watch is difficult because there are two pieces to its behavior when changes are made in the targeted state. The normal state still runs and then the mod_watch function runs in addition when there are changes. So which run should prereq trigger from? Either way you choose, someone else will likely argue for the other. =P

@basepi basepi added severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around Core relates to code central or existential to Salt P3 Priority 3 P4 Priority 4 and removed severity-low 4th level, cosemtic problems, work around exists P3 Priority 3 labels Apr 16, 2015
@stale
Copy link

stale bot commented Oct 27, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior Core relates to code central or existential to Salt P4 Priority 4 severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around stale
Projects
None yet
Development

No branches or pull requests

3 participants