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

Refactoring onlyif/unless - generalized with fallback? #55938

Closed
waynew opened this issue Jan 22, 2020 · 2 comments · Fixed by #55974
Closed

Refactoring onlyif/unless - generalized with fallback? #55938

waynew opened this issue Jan 22, 2020 · 2 comments · Fixed by #55974
Assignees
Labels
Bug broken, incorrect, or confusing behavior severity-high 2nd top severity, seen by most users, causes major problems ZRelease-Sodium retired label
Milestone

Comments

@waynew
Copy link
Contributor

waynew commented Jan 22, 2020

As I was going through the release notes for Neon/3000 I came across a feature that I thought was missing, but it turned out that there were just some issues with the way a few states already had onlyif/unless implementations.

This causes some issues with the new functionality to allow states in the onlyif/unless declarations.

It would be great if we could figure out a clean way to refactor those so they take advantage of that new functionality, but we don't break backwards compatibility. Though it may be worth breaking that compatibility in favor of having a unified onlyif/unless flow 🤔

In any case, the current codebase won't support the enhancements for a couple of fairly major states, such as cmd and file.

Example:

do_a_thing:
  cmd.run:
    - name: echo "what?"
    - onlyif:
      - fun: file.file_exists
        path: /tmp/fnord.txt

Fails like this:

local:
----------
          ID: do_a_thing
    Function: cmd.run
        Name: echo "what?"
      Result: True
     Comment: onlyif condition is false: OrderedDict([('fun', 'file.file_exists'), ('path', '/tmp/fnord.txt')])
     Started: 16:32:53.827416
    Duration: 157.563 ms
     Changes:   
Summary for local
------------
Succeeded: 1
Failed:    0
------------
Total states run:     1
Total run time: 157.563 ms

Where:

do_a_thing:
  http.query:
    - name: https://correcthorsebatterystaple.waynewerner.com
    - status: 200
    - onlyif:
      - fun: file.file_exists
        path: /tmp/fnord.txt

works correctly.

@sagetherage
Copy link
Contributor

@waynew can you get this labeled, please, I will put it in for consideration for next major release.

@sagetherage sagetherage added Bug broken, incorrect, or confusing behavior severity-high 2nd top severity, seen by most users, causes major problems labels Jan 24, 2020
@sagetherage sagetherage added this to the Approved milestone Feb 7, 2020
@sagetherage
Copy link
Contributor

@waynew looks like Christian has another issue and PR that is similar here - can you take a look and see if this is something we can pull into the Sodium?

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 severity-high 2nd top severity, seen by most users, causes major problems ZRelease-Sodium retired label
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants