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

RFC: create state.exists #45730

Merged
merged 9 commits into from Mar 5, 2018
Merged

RFC: create state.exists #45730

merged 9 commits into from Mar 5, 2018

Conversation

@mchugh19
Copy link
Contributor

@mchugh19 mchugh19 commented Jan 27, 2018

What does this PR do?

A hack which duplicates the state.show_sls to return True or False. Because states can live in gitfs, a simple file.exists isn't enough to support this need.

See this top file where states are assigned from a roles grain.
States are assigned to hosts, based on the roles listed in the roles grain. Without any other check, any roles in the grain but missing accompanying state files, will cause highstate to fail. By wrapping the role assignment in top with a check to validate it exists, these failures can be avoided.

# cat top.sls
base:
  '*':
    - common

  {% set roles = salt['grains.get']('roles',[]) -%}
  {% for role in roles -%}
  {% if salt['state.exists']('roles.{0}'.format(role)) -%}
  'roles:{{ role }}':
    - match: grain
    - roles.{{ role }}
  {% endif -%}
  {% endfor -%}

# salt-call state.show_top
[ERROR   ] Template was specified incorrectly: False
local:
    ----------
    base:
        - common
# touch roles/saltmaster.sls
# salt-call state.show_top
local:
    ----------
    base:
        - common
        - roles.saltmaster

What issues does this PR fix or reference?

Intent was to cover use cases such as #16687
It would be better to add functionality like ignore_missing to states, but how to do so was not immediately obvious.

Tests written?

No

Commits signed with GPG?

No

@rallytime rallytime requested a review from Feb 8, 2018
Copy link
Collaborator

@terminalmage terminalmage left a comment

Duplicating all of the code in show_sls is not necessary here. If there were errors, a list is returned, if not, a dict is returned. Therefore, this function can re-written like so:

return isinstance(
    show_sls(mods, test=test, queue=queue, **kwargs),
    dict
)

Also, I would prefer the name of the function be changed to sls_exists. This leaves open the option later on of adding a function to check if a given ID exists within a specified SLS file, and naming that function id_exists.

@mchugh19
Copy link
Contributor Author

@mchugh19 mchugh19 commented Feb 10, 2018

Fixed and renamed to sls_exists. Also added id_exists, and unit tests for both.

@rallytime rallytime requested a review from Feb 12, 2018
Copy link
Collaborator

@terminalmage terminalmage left a comment

A few docstring changes need to be made, and the id_exists function doesn't quite do what I was describing earlier. I've left an explanation and some example code to get you pointed in the right direction.

def sls_exists(mods, test=None, queue=False, **kwargs):
'''
Tests for the existance the of a specific sls or list of sls files on the
master. Similar to show_sls, rather than returning state details, returns
Copy link
Collaborator

@terminalmage terminalmage Feb 13, 2018

This can be improved by making show_sls a direct link to the docs for that function. For example, instead of "show_sls", you can do this:

:py:func:`state.show_sls <salt.modules.state.show_sls>`

When our docs are built, state.show_sls will link directly to the docs for that function.

Copy link
Contributor Author

@mchugh19 mchugh19 Feb 17, 2018

Done

for state_id in ids:
if state_id not in state_ids:
return False
return True
Copy link
Collaborator

@terminalmage terminalmage Feb 13, 2018

I think there was a misunderstanding of my earlier comment. When I referenced the concept of an "ID", I was talking about an individual state within an SLS file. The ID would be the top-level declaration for a given state. For instance, given the below SLS:

run_date_cmd:
  cmd.run:
    - name: date

zsh:
  pkg.installed

The IDs would be run_date_cmd and zsh.

Though I haven't tested this at all, the following code should work (or at least get you most of the way there):

if isinstance(ids, six.string_types):
    ids = ids.split(',')
ids = set(ids)

sls_ids = set(x['__id__'] for x in
              show_low_sls(mods, test=test, queue=queue, **kwargs))

return ids.issubset(sls_ids)

state.show_low_sls gives you a list of what we call the "low chunks", each chunk being a dictionary representing a single state, so if all of the passed IDs exist, then they should be a subset of all of the IDs in the specified SLS files.

Note that you'd need to rewrite the test as well.

Copy link
Contributor Author

@mchugh19 mchugh19 Feb 17, 2018

I believe the described functionality is what was implemented. The line
state_ids = show_sls(mods, test=test, queue=queue, **kwargs).keys()
causes the state to be rendered and the top level key names gathered. Thus, the state_ids variable should be a list of the state IDs.

Sounds like using _id_ key from lowstate might be a superior method, so I'll take a look.

Copy link
Contributor Author

@mchugh19 mchugh19 Feb 17, 2018

Done

.. versionadded:: Fluorine
saltenv
Specify a salt fileserver environment to be used when applying states
Copy link
Collaborator

@terminalmage terminalmage Feb 13, 2018

We're not actually applying states here, it would be more accurate to say:

        Specify a salt fileserver environment from which to look for the SLS
        files specified in the ``mods`` argument.

Copy link
Contributor Author

@mchugh19 mchugh19 Feb 17, 2018

Corrected

'''
Tests the for a specific id or list of ids within a given state on the
master. Similar to sls_exists, returns True or False. The default
environment is ``base``, use ``saltenv`` to specify a different environment.
Copy link
Collaborator

@terminalmage terminalmage Feb 13, 2018

This would be better worded as:

    Tests for the existence of a specific ID or list of IDs within the
    specified SLS file(s). Similar to :py:func:`state.sls_exists
    <salt.modules.state.sls_exists>`, returns True or False. The default
    environment is base``, use ``saltenv`` to specify a different environment.

Copy link
Contributor Author

@mchugh19 mchugh19 Feb 17, 2018

Done

.. versionadded:: Fluorine
saltenv
Specify a salt fileserver environment to be used when applying states
Copy link
Collaborator

@terminalmage terminalmage Feb 13, 2018

We're not actually applying states here, it would be more accurate to say:

        Specify a salt fileserver environment from which to look for the SLS
        files specified in the ``mods`` argument.

Copy link
Contributor Author

@mchugh19 mchugh19 Feb 17, 2018

Corrected

@@ -1880,6 +1880,59 @@ def show_sls(mods, test=None, queue=False, **kwargs):
return high_


def sls_exists(mods, test=None, queue=False, **kwargs):
'''
Tests for the existance the of a specific sls or list of sls files on the
Copy link
Collaborator

@terminalmage terminalmage Feb 13, 2018

SLS should be all-capitals when referring to an SLS file.

Copy link
Contributor Author

@mchugh19 mchugh19 Feb 17, 2018

Corrected

Improve id_exists by using show_low_sls and checking __id__ keys.
@mchugh19
Copy link
Contributor Author

@mchugh19 mchugh19 commented Feb 17, 2018

Thanks @terminalmage. Suggestions/corrections applied.

@mchugh19
Copy link
Contributor Author

@mchugh19 mchugh19 commented Feb 28, 2018

I think all of the requested updates have been submitted. Is there anything outstanding?

@rallytime rallytime requested review from terminalmage and Feb 28, 2018
Copy link
Collaborator

@terminalmage terminalmage left a comment

This looks good, thanks for your patience as we've worked through this.

salt '*' state.id_exists create_myfile,update_template filestate saltenv=dev
'''
if isinstance(ids, six.string_types):
ids = ids.split(',')
Copy link
Contributor

@isbm isbm Feb 28, 2018

Please use salt.utils.args.split_input instead of this:

def id_exists(...
    sls_ids = set(x['__id__'] for x ...
    return set(salt.utils.args.split_input(ids)).issubset(sls_ids)

And it would be nice to others not to use one-char variables.

Copy link
Collaborator

@terminalmage terminalmage Feb 28, 2018

I don't mind the 1-char in a comprehension expression, to be honest. I don't think that needs to change.

@isbm is right about the split_input, we do have a helper for this. It could be done as

ids = salt.utils.args.split_input(ids)

@mchugh19
Copy link
Contributor Author

@mchugh19 mchugh19 commented Mar 1, 2018

Updated to use salt.utils.args.split_input. While there, I also updated the other uses of variable.split(',') in the file to keep everything consistent. Is that alright?

@isbm
Copy link
Contributor

@isbm isbm commented Mar 1, 2018

Updated to use salt.utils.args.split_input. While there, I also updated the other uses of variable.split(',') in the file to keep everything consistent. Is that alright?

Yes, this is alright. Almost. 😉 You just don't need to extra-check if isinstance... is string. Just split directly (and so remove all those checks by the way).

Thanks!

@mchugh19
Copy link
Contributor Author

@mchugh19 mchugh19 commented Mar 1, 2018

Ah, that makes sense. Done!

isbm
isbm approved these changes Mar 1, 2018
Copy link
Contributor

@isbm isbm left a comment

@rallytime rallytime merged commit c99dbd8 into saltstack:develop Mar 5, 2018
5 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants