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

[develop] Adding ability to disable requisites #49955

Merged
merged 8 commits into from Oct 23, 2018

Conversation

garethgreenaway
Copy link
Contributor

@garethgreenaway garethgreenaway commented Oct 9, 2018

What does this PR do?

Adding the ability to disable requisites during State runs.

What issues does this PR fix or reference?

N/A

Tests written?

Yes.

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@garethgreenaway garethgreenaway requested a review from a team as a code owner October 9, 2018 20:23
@ghost ghost requested review from a team October 9, 2018 20:23
@garethgreenaway
Copy link
Contributor Author

garethgreenaway commented Oct 9, 2018

Manual tests for disabling requisites:

Require State file:

vim-tiny:
  pkg.installed:
    - require:
      - file: /tmp/vimrc

/tmp/vimrc:
  file.managed:
    - source: salt://test/edit/vimrc

Require disabled:

disabled_requisites: require

Normal run:

local:
----------
          ID: /tmp/vimrc
    Function: file.managed
      Result: True
     Comment: File /tmp/vimrc is in the correct state
     Started: 15:23:22.162520
    Duration: 20.0000000186 ms
     Changes:   
----------
          ID: vim-tiny
    Function: pkg.installed
      Result: True
     Comment: All specified packages are already installed
     Started: 15:23:22.185465
    Duration: 89.999999851 ms
     Changes:   

Summary for local
------------
Succeeded: 2
Failed:    0
------------
Total states run:     2
Total run time: 110.000 ms

Disabled run:

local:
----------
          ID: vim-tiny
    Function: pkg.installed
      Result: True
     Comment: All specified packages are already installed
     Started: 15:24:22.655635
    Duration: 89.999999851 ms
     Changes:   
----------
          ID: /tmp/vimrc
    Function: file.managed
      Result: True
     Comment: The file /tmp/vimrc is in the correct state
     Started: 15:24:22.747198
    Duration: 20.0000000186 ms
     Changes:   

Summary for local
------------
Succeeded: 2
Failed:    0
------------
Total states run:     2
Total run time: 110.000 ms

Require_In State File:

vim-tiny:
  pkg.installed

/tmp/vimrc:
  file.managed:
    - source: salt://test/edit/vimrc
    - require_in:
      - pkg: vim-tiny

Require disabled:

disabled_requisites: require_in

Normal Run:

local:
----------
          ID: /tmp/vimrc
    Function: file.managed
      Result: True
     Comment: The file /tmp/vimrc is in the correct state
     Started: 16:12:41.378112
    Duration: 10.0000000093 ms
     Changes:   
----------
          ID: vim-tiny
    Function: pkg.installed
      Result: True
     Comment: All specified packages are already installed
     Started: 16:12:41.393692
    Duration: 90.0000000838 ms
     Changes:   

Summary for local
------------
Succeeded: 2
Failed:    0
------------
Total states run:     2
Total run time: 100.000 ms

Disabled_run:

[WARNING ] The require_in requisite has been disabled, Ignoring.
local:
----------
          ID: vim-tiny
    Function: pkg.installed
      Result: True
     Comment: All specified packages are already installed
     Started: 16:13:17.053604
    Duration: 90.0000000838 ms
     Changes:   
----------
          ID: /tmp/vimrc
    Function: file.managed
      Result: True
     Comment: The file /tmp/vimrc is in the correct state
     Started: 16:13:17.149302
    Duration: 10.0000000093 ms
     Changes:   

Summary for local
------------
Succeeded: 2
Failed:    0
------------
Total states run:     2
Total run time: 100.000 ms

Copy link
Contributor

@isbm isbm left a comment

Choose a reason for hiding this comment

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

@garethgreenaway I would definitely start with some more unit tests in tests/unit/test_state.py for specific functions in the state compiler. The def requisite_in(self, high): deserves nearly a whole class of unit tests on its own due to its enormous size and complexity. And probably refactoring...

Copy link
Contributor

@terminalmage terminalmage left a comment

Choose a reason for hiding this comment

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

Perhaps the best way to test this is to unit test some of the intermediate functions in the state compiler. tests/unit/test_state.py does this, see test_render_error_on_invalid_requisite for an example.

This approach would require you to provide the high data, and then then call the salt.state.State object's requisite_in method.

I wonder also if it would be a good idea to add a message to the error list when a disabled requisite is used, which would cause the states not to run and return an error message.

@gtmanfred
Copy link
Contributor

@cachedout
Copy link
Contributor

@terminalmage and @isbm Re-review requested.

Copy link
Contributor

@cachedout cachedout left a comment

Choose a reason for hiding this comment

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

LGTM. I think this fits the bill.

@isbm
Copy link
Contributor

isbm commented Oct 16, 2018

I would still write a bit more UTs though.

@rallytime
Copy link
Contributor

@garethgreenaway There is a merge conflict here now unfortunately. Can you rebase and resolve the conflicts?

@cachedout cachedout merged commit 2a23adf into saltstack:develop Oct 23, 2018
@waynew waynew added this to PR needs port to master in PRs to port to master Oct 24, 2019
dwoz added a commit that referenced this pull request Apr 23, 2020
@sagetherage sagetherage moved this from PR needs port to master to PR has port to master in PRs to port to master Apr 23, 2020
@sagetherage sagetherage added code-jam has master-port port to master has been created labels Apr 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-jam has master-port port to master has been created
Projects
PRs to port to master
  
PR has port to master
Development

Successfully merging this pull request may close these issues.

None yet

7 participants