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

saltcheck module updates #53024

Closed
wants to merge 5 commits into from
Closed

Conversation

lomeroe
Copy link
Contributor

@lomeroe lomeroe commented May 14, 2019

What does this PR do?

Update saltcheck to:

  1. not require synchronizing the entire fileserver down to the minion,
    tests will be searched and generated from the sls low data

  2. allow assertion checking multiple levels into a returned dictionary

  3. allow custom saltcheck test locations inside a state/formula's folder

  4. provide more information on invalid tests

What issues does this PR fix or reference?

N/A

Previous Behavior

  1. Entire fileserver tree was synchronized to the minion to search for tests
  2. only a first level of a dictionary returned by a module could have a value tested
  3. saltcheck files were required to exist in a specific location in the formula/state folder
  4. Why a test was marked "invalid" was not presented

New Behavior

  1. Only the test files are synchronized to the client, this is done by generating the sls low data for the state/highstate operation and then caching only the test files
  2. A dictionary can be passed to the assertion_section to provide the nested key to be tested
  3. A minion configuration setting of saltcheck_test_location can be used to specify the relative path to the tests files from the state/formula's folder
  4. Why a test was marked invalid is reported as an error in the minion log

Tests written?

No

Commits signed with GPG?

No

Please review Salt's Contributing Guide for best practices.

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

lomeroe added 3 commits May 14, 2019 12:12
1) not require synchronizing the entire fileserver down to the minion,
tests will be searched and generated from the sls low data

2) allow assertion checking multiple levels into a returned dictionary

3) allow custom saltcheck test locations inside a state/formula's folder
Remove test_update_master_cache test as this function is no longer in
the module
@mchugh19
Copy link
Contributor

Thanks for working on the saltcheck module! From your list of fixes:

  1. We need to be a bit careful here. In Saltcheck salt-ssh support #52541 this was converted to use cp.cache_dir to get the whole state directory rather than the sync the entire fileserver. The state directory was used rather than just the individual states and tests because jinja rendering can be used in a test file and thus could include a map.jinja or other includes.
  2. This is also corrected in Saltcheck salt-ssh support #52541 though the use of salt.utils.data.traverse_dict_and_list
  3. Great!
  4. Also great!

@lomeroe
Copy link
Contributor Author

lomeroe commented May 15, 2019

@mchugh19 -- doh, I didn't look for pending PRs that made any updates :/

on 1), I think we're basically doing the same thing, I'm just looking for any tst files to cache based on the sls low data from rendering the state -- as long as we're not caching the whole fileserver tree, I'm happy -- admittedly I haven't done any testing w/my implementation with salt-ssh, but it does allow for including map/jinja/default/etc files in the tst files/etc as the rendering engine will cache any imports/etc when the tst files are rendered

on 2) that is much better than what I did, I should have looked through the utils for a better way to do it

3 & 4 shouldn't be a big deal to convert into your implementation when it is merged

@mchugh19
Copy link
Contributor

mchugh19 commented May 15, 2019

it does allow for including map/jinja/default/etc files in the tst files/etc as the rendering engine will cache any imports/etc when the tst files are rendered

This is the bit I was wondering about, as that's not been my experience. Can you verify if a tst like the following works for you with a clean cache directory? This is why I used the logic of running cp.cache_dir on the entire state directory of each needed state.

I tried it myself, and it does indeed work as you've described!

{% from "ntp/map.jinja" import ntp with context %}

check_installed:
  module_and_function: pkg.version
  args:
    - {{ ntp.client }}
  assertion: assertNotEmpty

As for the logic of running tests based on the output of low state in order to better associate tests with states using includes, that's great!

@mchugh19 mchugh19 mentioned this pull request May 16, 2019
@lomeroe
Copy link
Contributor Author

lomeroe commented May 20, 2019

closing in favor of #53068

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants