Skip to content

Fix behavior for "onlyif/unless" conditional when multiple declarations - #59345

Merged
Ch3LL merged 9 commits into
saltstack:masterfrom
meaksh:master-fix-onlyif-and-unless-behaviors
Feb 11, 2021
Merged

Ch3LL merged 9 commits into
saltstack:masterfrom
meaksh:master-fix-onlyif-and-unless-behaviors

Conversation

@meaksh

@meaksh meaksh commented Jan 22, 2021 •

Copy link
Copy Markdown
Contributor

What does this PR do?

This PR fixes a problem with the logic for onlyif and unless state conditionals that makes states with multiple "onlyif" or "unless" conditionals to only take care of the latest condition to compute the results.

This causes a wrong behavior when applying states like the following:

 package_should_not_be_installed_1:
  pkg.installed:
    - name: non-existant-1
    - onlyif:
      - /bin/true
      - /bin/false

package_should_not_be_installed_2:
  pkg.installed:
    - name: non-existant-2
    - onlyif:
      - /bin/false
      - /bin/true

package_should_not_be_installed_3:
  pkg.installed:
    - name: non-existant-3
    - onlyif:
      - /bin/false
      - /bin/false

NOTE: This bug seems to be present since Salt 3000.

What issues does this PR fix or reference?

Fixes: #58085

Previous Behavior

Without this PR, the output from the above state execution using test=True is not behaving properly:

local:
----------
          ID: package_should_not_be_installed_1
    Function: pkg.installed
        Name: non-existant-1
      Result: True
     Comment: onlyif condition is false
     Started: 12:02:40.287427
    Duration: 457.454 ms
     Changes:   
----------
          ID: package_should_not_be_installed_2
    Function: pkg.installed
        Name: non-existant-2
      Result: None
     Comment: The following packages would be installed/updated: non-existant-2
     Started: 12:02:40.745221
    Duration: 16922.635 ms
     Changes:   
----------
          ID: package_should_not_be_installed_3
    Function: pkg.installed
        Name: non-existant-3
      Result: True
     Comment: onlyif condition is false
     Started: 12:02:57.668110
    Duration: 20.366 ms
     Changes:   

Summary for local
------------
Succeeded: 3 (unchanged=1)
Failed:    0
------------
Total states run:     3
Total run time:  17.400 s

Notice the conditions for package_should_not_be_installed_2 didn't work as expected an the package was set to be installed even if the first of its onlyif conditions returned False

New Behavior

After this PR, the output is the expected one:

local:
----------
          ID: package_should_not_be_installed_1
    Function: pkg.installed
        Name: non-existant-1
      Result: True
     Comment: onlyif condition is false
     Started: 12:01:09.294615
    Duration: 680.444 ms
     Changes:   
----------
          ID: package_should_not_be_installed_2
    Function: pkg.installed
        Name: non-existant-2
      Result: True
     Comment: onlyif condition is false
     Started: 12:01:09.975409
    Duration: 13.693 ms
     Changes:   
----------
          ID: package_should_not_be_installed_3
    Function: pkg.installed
        Name: non-existant-3
      Result: True
     Comment: onlyif condition is false
     Started: 12:01:09.989453
    Duration: 15.594 ms
     Changes:   

Summary for local
------------
Succeeded: 3
Failed:    0
------------
Total states run:     3
Total run time: 709.731 ms

Merge requirements satisfied?

Commits signed with GPG?

Yes

@meaksh
meaksh requested a review from a team as a code owner January 22, 2021 12:07
@meaksh
meaksh requested review from Ch3LL and removed request for a team January 22, 2021 12:07
Comment thread tests/unit/test_state.py

@Ch3LL Ch3LL left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just need to add a changelog and have one question in the PR. Thanks :)

@meaksh

meaksh commented Jan 28, 2021

Copy link
Copy Markdown
Contributor Author

Changelog pushed.

Thanks for the review @Ch3LL :)

@Ch3LL
Ch3LL requested a review from garethgreenaway February 9, 2021 19:03
@Ch3LL Ch3LL added the Aluminium Release Post Mg and Pre Si label Feb 9, 2021
@Ch3LL
Ch3LL merged commit 718249a into saltstack:master Feb 11, 2021
@meaksh
meaksh deleted the master-fix-onlyif-and-unless-behaviors branch February 11, 2021 12:34
@twangboy twangboy mentioned this pull request Feb 11, 2021
1 of 3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Aluminium Release Post Mg and Pre Si

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] onlyif should run only if ALL commands return true

3 participants