@saltstack/team-core Thoughts?

We have gone back and forth on this one a lot over the years. The core argument has been that the changes dict should only have changes in it if changes actually happened. Thats why it keeps getting pulled out. The main argument is that this is the expected behavior.

The counter argument is that this is some very valuable information to have from a test=True run and that it should be made available.

The counter/counter to that is that we don't have the ability to get predictive changes out of all states, therefore the output becomes inconsistent.

Finally, it can also be argued that if the result is None then we should know for sure that we did not make changes, so any changes in the changes dict can be seen as predictive.

Brief discussion between @thatch45 @waynew and myself, we concluded that the diff should be shown and we'll look to see what changed from 2018.3.x to 2019.2.x to disable this.

Agreed, and when we get it back, lets add a fat comment in there to keep it :)

Thanks for those informations and for your reactivity.

Even if test=True isn't perfect and could miss some changes (generally due to triggers like watch / watch_in etc.), it is a huge help for the one who needs to check some particular changes instead of blindly applying states and just hope changes will be good.
A note could be displayed on test=True telling the user that changes are juste there for information and can be wrong in certain cases.

I'm a fan of adding a note in there, maybe something like Comment: File /tmp/dest_file.txt updated. Note: Actual changes may be different due to other states.

I like it, maybe say:
Note: No changes made, actual changes may be different due to other states.

If it's of any help, from our tests, looks like this commit might be the culprit:

98c61ae

#44353

I would only add that a) This is a major feature when trying to merge a Salt state into systems that have not previously been managed by Salt and have years of manual changes in them. One never knows what setting you're going to be accidentally removing. b) If test=true isn't the grammitically correct way this is envisioned then adding a state.test that does the same thing is perhaps more grammatically correct?

I can confirm reverting the change @fedepires mentions and restarting my remote minion "fixes" the issue and shows the diffs when running state.apply with test=true

@fedepires Nice find :) (and helpful for those who need it.

@rhavenn At least from those involved in the minor discussion we had, we didn't have strong feelings about calling it something else. My cursory search didn't find anything to the contrary. I'd say that my personal preference would be to fix this back (with the aforementioned comment), with the idea that test=True is basically equivalent to git's dry-run flag.

Maybe if someone really feels strongly about the semantics/grammar they could submit a PR to introduce a dry-run flag and re-un-restore the behavior for test=True :)

To add a last thought to the @rhavenn 's comment, currently, if you misspell "test=True", the state will be executed without any error or message telling that we passed an unknown argument, which is very dangerous and confusing.

Then, replacing test=True by a special state.test (or maybe state.dryrun) would be a good thing I think,

Hello,

I agree that this is a terrible regression, it makes it impossible to actually see what changes are going to be made in test mode, almost defeating the entire purpose of the test mode (If you can't see potential changes, then the test mode becomes useful only as a syntax test, to check for errors in the states). We use test mode to capture differences in a nightly audit also which has now become useless without the details of the differences.

Although the change described by @fedepires and tested by @rhavenn supposedly fixes it, there is another alternative fix, potentially more suitable one, because the fix below restores the behaviour for handling all pchanges data from different states, whereas the fix above, only restores the changes displayed within the 'file' module. That fix also introduces the 'mixing' of 'changes' and 'pchanges', which people are concerned with above. However the fix below only affects the output produced (choosing to display pchanges if it is available).

The code in highstate.py, used to print 'pchanges' if it was present and there were no 'changes' but there are 'pchanges'.

This handling was removed in commit 98cfa1f "recursion for orchestration" (file salt/output/highstate.py, line 226), and may have simply been an oversight.

this fix is as follows (simply restoring the removed logic) from that commit

Around line 246 of salt/output/highstate.py, in salt version 2019.2.0, within function _format_host :

    schanged, ctext = _format_changes(ret['changes'])
    + if not ctext and 'pchanges' in ret:
    +     schanged, ctext = _format_changes(ret['pchanges'])
    nchanges += 1 if schanged else 0

The @cjsin 's patch works perfectly for me. Thanks!

Can confirm, this works great. Is there a plan to merge this change into master? This was hands down the most valuable feature of salt (and of configuration management tools in general) so it would be great to have it back without manually patching.

@dpkirchner From what I can tell this should be landing in 2019.2.1, but @garethgreenaway can correct me if I'm wrong.

Correct, #51992 is labeled to be merged for the 2019.2.1 release.

Hi,
this issue seems to pop out again with version 2019.2.5

@marceliq can you give your set up and Salt version report, please?

@sagetherage sorry for late answer, here it is:

Salt is installed from pip.

salt-call --versions-report
Salt Version:
           Salt: 2019.2.5
 
Dependency Versions:
           cffi: 1.13.2
       cherrypy: 17.4.0
       dateutil: 2.8.1
      docker-py: 1.10.6
          gitdb: 2.0.6
      gitpython: 2.1.15
          ioflo: Not Installed
         Jinja2: 2.10.3
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: 0.32.0
           Mako: 1.1.0
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: 2.19
       pycrypto: 3.9.4
   pycryptodome: 3.9.4
         pygit2: Not Installed
         Python: 2.7.17 (default, May 14 2020, 06:37:18)
   python-gnupg: 0.4.6
         PyYAML: 5.2
          PyZMQ: 18.1.1
           RAET: Not Installed
          smmap: 3.0.4
        timelib: 0.2.4
        Tornado: 5.1.1
            ZMQ: 4.3.2
 
System Versions:
           dist: centos 7.7.1908 Core
         locale: UTF-8
        machine: x86_64
        release: 3.10.0-1062.el7.x86_64
         system: Linux
        version: CentOS Linux 7.7.1908 Core

Note to mention, that file.serialize shows diffs, but file.manage dont.

@marceliq what state are you using and where exactly are you see this happening? I want to gather as much information as I can so we can really narrow down where this is still occurring and get it corrected. Thank you!

removing this from a release cycle until unblocked

@sagetherage what is the blocker on this bug? I would love to see it fixed. Anything I can do to help unBlock it?

@wwalker @sagetherage I'm going to dig in here to sort out where the issue stands.

AFAICT, this is resolved in minions ≥ 2019.2.1, as noted in #51932 (comment).

It's possible the issue is still being seen based on an assumption that the master version matters here. It does not, the behaviour here is fully driven by the minions. Here's an example with a 3002.2 master with a 2018.3.5 minion (old) and a 3002.2 minion (master1).

# salt '*' state.apply gh51932.test test=True
old:
----------
          ID: /tmp/dest_file.txt
    Function: file.managed
      Result: None
     Comment: The file /tmp/dest_file.txt is set to be changed
     Started: 23:35:59.480332
    Duration: 12.141 ms
     Changes:   

Summary for old
------------
Succeeded: 1 (unchanged=1)
Failed:    0
------------
Total states run:     1
Total run time:  12.141 ms

master1:
----------
          ID: /tmp/dest_file.txt
    Function: file.managed
      Result: None
     Comment: The file /tmp/dest_file.txt is set to be changed
              Note: No changes made, actual changes may
              be different due to other states.
     Started: 23:36:00.331591
    Duration: 8.46 ms
     Changes:   
              ----------
              diff:
                  --- 
                  +++ 
                  @@ -1 +1 @@
                  -dest
                  +source

Summary for master1
------------
Succeeded: 1 (unchanged=1, changed=1)
Failed:    0
------------
Total states run:     1
Total run time:   8.460 ms