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

allow result parsing in unless/onlyif #57504

Merged
merged 3 commits into from Sep 8, 2020
Merged

Conversation

mchugh19
Copy link
Contributor

What does this PR do?

Adds get_return key to onlyif and unless requisites. This key is used to determine the value to parse for modules which return deep data structures.

What issues does this PR fix or reference?

Fixes: #57470

Previous Behavior

Given the state:

test:
  test.nop:
    - name: foo
    - unless:
      - fun: test.arg
        kwarg:
          deep: False

The execution module returns kwargs: {deep: False}. We'd like to evaluate that key for the onlyif or unless behavior, but it is not possible. Having a result returned at all is evaluated as True, and thus the state does not run.

New Behavior

Given the state:

test:
  test.nop:
    - name: foo
    - unless:
      - fun: test.arg
        kwarg:
          deep: False
        get_return: kwargs:deep

The False return of the module can now be detected and evaluated by the unless/onlyif requisites, and the state is run.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

No

@mchugh19 mchugh19 requested a review from a team as a code owner May 30, 2020 07:21
@ghost ghost requested review from garethgreenaway and removed request for a team May 30, 2020 07:21
Copy link
Contributor

@Oloremo Oloremo left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

It indeed makes things better than there were yet I think it doesn't solve all problems, please take a look at my comments

result = self._run_check_function(entry)
if get_return:
result = salt.utils.data.traverse_dict_and_list(result, get_return)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% sure how this will work if we have duplicated key names in nested data structure? Like
get_return: result

and the return data:"

result:
   - result: True

The last will be fecthed or the first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR uses the same code salt does to walk pillar or any other nested data structure.

Here's a custom module which outputs a deep dict like you've described:

# salt-call --local custom.deep_report
local:
    ----------
    result:
        ----------
        result:
            True

This result can be fetched with a test like:

test:
  test.nop:
    - name: foo
    - unless:
      - fun: custom.deep_report
        get_return: result:result

So just like pillar or other references to deep keys, here you'd use result:result to get at the value under the two result keys.

result = self._run_check_function(entry)
if get_return:
result = salt.utils.data.traverse_dict_and_list(result, get_return)
if self.state_con.get("retcode", 0):
_check_cmd(self.state_con["retcode"])
elif result:
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think it's very questionable even with that patch.

For example, we can have a string "false" in the result and it will be evaluated as True since it's non-empty string.

I think to make it really useful we need a return check, like and eval or assert.
Also, we may need a multiple checks actually, for example:

    test:
      test.nop:
        - name: foo
        - onlyif:
          - fun: consul.get
            consul_url: http://127.0.0.1:8500
            key:  not-existing
          - get_return: res
            when:
              - type(res) is dict
              - res.get('Result', False) is True             

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an issue. There isn't a standard for execution module return success, so parsing possible results would really mean recreating the assert structure of saltcheck where you would need to describe text equals/doesn't equal, exists/doesn't exist, is true/is false, etc.

While very possible, I think that might be a bit much for the definition of requisites as advanced parsing should really occur in states. If it's agreed that more advanced logic should be part of the requisite system here, I'm happy to add that to the PR as well. Heck, we could just use saltcheck for the test itself and only have unless/onlyif just have to parse saltcheck's Pass/Fail output:

test:
  test.nop:
    - name: foo
    - unless:
      - fun: saltcheck.run_test
        test:
          module_and_function: consul.get
          assertion: assertTrue
          assertion_section: res

Copy link
Contributor

Choose a reason for hiding this comment

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

that is actually interesting!

Copy link
Contributor

Choose a reason for hiding this comment

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

We have an initiative right now to start standardizing returns from execution modules. Though it's going to take some time to complete those efforts. We're aiming to get some validation in place during MG that can start warning of non-standard returns.

Akm0d
Akm0d previously approved these changes Jun 1, 2020
@sagetherage sagetherage added Magnesium Mg release after Na prior to Al Feature new functionality including changes to functionality and code refactors, etc. State-Compiler labels Jul 22, 2020
@Oloremo
Copy link
Contributor

Oloremo commented Aug 25, 2020

@mchugh19 mind to rebase this so it could be considered for merging?

@mchugh19
Copy link
Contributor Author

@Oloremo updated to hopefully satisfy pre-commit. Since it just ran a py2 cleanup script against the very critical state.py, getting all eyes on it would be nice.

dwoz
dwoz approved these changes Sep 8, 2020
@dwoz dwoz merged commit a801c8d into saltstack:master Sep 8, 2020
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature new functionality including changes to functionality and code refactors, etc. has-failing-test Magnesium Mg release after Na prior to Al State-Compiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] Add a return evaluation to unless & onlyif requisites
5 participants