Do not fail states during mock test mode

[vutny]

The key purpose of this PR is to be able to test and debug rendering postgres states.
It makes mocked test run (salt-call state.apply postgres mock=True) return success if Pillar data, defaults and Jinja logic are syntactically valid.
The only problem with postgres.manage SLS is that the states would always fail until PG client binaries would be installed.

This could be simply fixed by forcing postgres-reload-modules state always produce changes and subscribe on those. Since "mocked" test does not generate any changes, we will get success. That means the code is valid, although may be logically incorrect.

@javierbertoli Would you be able to review this, please? Thank you in advance.

[vutny]

@noelmcloughlin Maybe you could look on this as well? Thanks.

[noelmcloughlin]

Here is my review.

You are replacing test.nop (a successful state that does nothing) with test.succeed_with_changes (a successful state with changes=True) for state postgres-reload-modules and appending same as onchanges prerequisite in macros.sls. This makes require.test.postgres-reload-modules parameters superfluous in manage.sls thus you have removed same.

This is nice improvement, well implemented, which can be merged safely.

Thx @vutny

[noelmcloughlin]

@aboe76 could you merge if happy. Thx.

[Magomogo]

Today I've updated postgres-formula in my project and found that state postgres-reload-modules is always executed with no reason. I suspect this MR is the cause of the issue. Am I right? Why you did that?

[vutny]

Hi @Magomogo

In general, a good practice to follow is when Salt states do nothing, they should not report any changes. However, sometimes it is unavoidable for some states to return changes each run.

In this PR I tried to overcome limitation of Salt's postgres state modules. They are fail to load when no PostgreSQL binaries would installed in the system $PATH at the time of the state rendering.
That postgres-reload-modules state is actually a test state, which does nothing by itself, but the reload_modules parameter forces it additionally to discover newly installed PostgreSQL client and tools.
The problem is that when the state does not produce changes, we can't reliably use Salt's build-in mock interface for testing state rendering and order of execution with mock=True command-line option. On a fresh installation all states which depend on psql presence (for example) would fail even if we explicitly told Salt to mock them.

This is rather the problem of postgres state modules implementation though... But, by forcing the postgres-reload-modules state to return some changes and subscribe to that event in later states, we are able to get a clean test run in clean environment. It really helps to debug and test even changes in Pillar.

If you're concerned with the unexpected state changes, you may apply postgres.manage state conditionally (based on Pillar) from the state top file, in contrast of running whole postgres state tree.
Also I think I could try to make this behavior configurable or set a special grain. Would you mind to raise an issue here if it's really that necessary to have?

And thanks for the question, I needed to explain the context behind the PR better.

[Magomogo]

@vutny thanks for the detailed explanation.

However dummy states always returning the changes is a real problem IMHO. Will raise an issue.