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

Add state.show_states call. #44475

Merged
merged 2 commits into from Jan 17, 2018

Conversation

Projects
None yet
6 participants
@epcim
Copy link
Contributor

commented Nov 10, 2017

What does this PR do?

list individual sls state's to be applied.

What issues does this PR fix or reference?

I found the function under this discussing: #6265
Thx, @cenkalti for writing it.

Tests written?

No

Commits signed with GPG?

Yes

How this can be used

One can get the list of these low state's to be applied and then query how many of them would perform a change on a node. For example, this is today script (https://github.com/salt-formulas/salt-formulas-scripts/blob/master/salt-state-apply-trend.sh) I am using.

(as up to now we don't have any statistics of this kind available (feel free to correct me)).

@garethgreenaway

This comment has been minimized.

Copy link
Member

commented Nov 10, 2017

@epcim Great addition! Thanks! Would you mind adding some unit tests to the PR for this new function? Thanks!

@@ -1352,6 +1353,20 @@ def show_state_usage(queue=False, **kwargs):
return ret


def show_states():
"""Returns the list of states that will be applied on highstate."""

This comment has been minimized.

Copy link
@cachedout

cachedout Nov 10, 2017

Collaborator

Could you please break these out into three lines and change the double-quotes to single-quotes? Thanks.

This comment has been minimized.

Copy link
@rallytime

rallytime Nov 10, 2017

Contributor

This could also use a .. versionadded:: Oxygen tag while you're editing this.

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Nov 10, 2017

@garethgreenaway @epcim Integration tests might be a better choice here, IMHO.

@epcim epcim force-pushed the epcim:add-showstates branch 3 times, most recently from bc3551f to 1be19e9 Nov 13, 2017

@epcim

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2017

I am not satisfied with the speed. Anyone know, how we could modify compile_high_data to speed up the results, as we are only interested in sorted list of __sls__ values...

@rallytime rallytime requested a review from garethgreenaway Dec 5, 2017

@garethgreenaway
Copy link
Member

left a comment

Looks good to me but @cachedout had requested integration tests in addition to or instead of the unit tests, so I'll defer.

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2017

Ah, yes. Integration tests would be good here @epcim. Please let us know if you need any help.

@epcim

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2017

@rallytime commented on Dec 5, 2017, 10:39 PM GMT+1:

Ah, yes. Integration tests would be good here @epcim. Please let us know if you need any help.

I dont have any idea how to write tests properly for that, as what it does it's 1:1 code from low_sls call (if I remember + additional 5 lines of loop in order to pick sls. That code is supposed to be already tested by mentioned method.

The only test I can imagine is that it returns a type list. As the in/out vlaues will be in the test call anyway modified to match. If you really need tests, can you post an update with some example that I could work on.

Worth if you point out why it;s failing..

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2018

@epcim Thank you so much for adding these tests. I just checked and they're passing nicely. The other failures here are not related.

@garethgreenaway Can you swing by here one more time when you get a moment?

@rallytime rallytime requested a review from garethgreenaway Jan 2, 2018

@epcim epcim force-pushed the epcim:add-showstates branch from a5f71b8 to e3c1050 Jan 3, 2018

@epcim

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2018

Squashed commit's in the last push.

  • removed the unsorted workflow with modified compile_high* as not faster.
@rallytime

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2018

Go Go Jenkins!

@epcim

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2018

Failures seems to me are not related to this patch..

@rallytime rallytime merged commit 9531fa1 into saltstack:develop Jan 17, 2018

4 of 10 checks passed

codeclimate 1 issue to fix
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #1352 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #18857 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #5858 — FAILURE
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #15346 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #21302 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #13722 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #18326 — SUCCESS
Details

rallytime added a commit to rallytime/salt that referenced this pull request Jul 3, 2018

Oxygen should be Fluorine in state.show_states addition
This feature was added in PR saltstack#44475, which is presently only in the
`develop` branch. Therefore, the `versionadded` tag should be
`Fluorine` instead of `Oxygen`.
@max-arnold

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2018

@epcim How this is different from state.show_top?

@cenkalti

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2018

@max-arnold show_top lists states only defined in top.sls whereas show_states lists all states that will be applied to minion.

In other words, if a state is included from another state, it will not be in show_top output but be in show_states output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.