More intelligent diffing in changes of file.serialize state

[anlutro]

I wanted some feedback on this minor feature before I tidy it up, write tests etc. - I've confirmed it works with a local checkout of the salt codebase.

Basically I have a JSON config file being generated by a Go application, and I want to manage parts of it with Salt. However, because Python and Go order their JSON differently (order of keys mostly), the diff output is always massive, making it impossible to see what changed.

I found salt.utils.dictdiffer which does exactly what I want in this scenario. I added an optional argument to file.serializer which switches the behaviour of the state's changes output.

Some problems/questions which I'd like feedback on:

• Should this behaviour be the default or is it best to keep it behind this optional argument? Is the name of the argument okay?
• The changes output is potentially misleading, because even if the serialized data doesn't change, the file's contents (indentation or whatever) may change, which could screw up onchanges etc.
• dictdiffer only works on dicts, but it's possible to serialize other data types such as lists.

[DmitryKuzmenko]

Basically I'm OK with the idea.

Should this behaviour be the default or is it best to keep it behind this optional argument?

False by default as for me, to not break this for others.

Is the name of the argument okay?

Maybe shorter? show_diff? But I like to hear an opinion of somebody else.

The changes output is potentially misleading, because even if the serialized data doesn't change, the file's contents (indentation or whatever) may change, which could screw up onchanges etc.

Maybe just document this point?

dictdiffer only works on dicts, but it's possible to serialize other data types such as lists.

Not sure it's a good idea but the simplest way is if it's not a dict make it a dict {'data': existing_data} or better implement a list diffing logic.

[anlutro]

Isn't show_diff reserved as a global argument meant to control whether any diff at all is shown? I think it sounds a little bit misleading anyway - if it's false, the diff is still shown, it's just shown in a different way.

[DmitryKuzmenko]

@anlutro I'm ok then. Sorry for misleading

[anlutro]

Cool. I will work on the list stuff as well as docs a bit later today, hopefully.

[rallytime]

I am OK with this change, but the current behavior should be the default. I'd also like @terminalmage to take a look here as well.

[rallytime]

This should be False by default to keep the previous behavior, yes?

[terminalmage]

I like anything that helps normalize changes. However, I don't think we need to hide this behavior behind an argument. I'm not sure I understand what changing the format of the diffs in the changes key of the return data is going to "break".

IMO this behavior should be the new default and we don't need to make it contingent on an argument passed to the state.

[rallytime]

OK, after talking to @terminalmage about this offline, I agree with him. We definitely need a mention of the change in the release notes though.

[anlutro]

Okay, cool. I just need to figure out how to do the diffing, then. I can't find anything in dictdiffer that does list diffs in particular, and the listdiffer module seems to be aimed at lists of dictionaries in particular. Will I need to implement it myself? What would the module name be, since listdiffer is already taken?

[anlutro]

On second thought, after considering how difficult smartly diffing lists is, maybe I should just not do that... But that might mean that data structures such as lists of dicts or lists of lists of dicts won't get smartly diffed.

Maybe I can simply wrap the dictdiff call in an if isinstance(data, dict) and call it a day?

[terminalmage]

@anlutro Can you elaborate on what you mean by list diffing? The recursive diffing in dictdiffer does show where lists differ.

To me this looks good as-is, but since I'm not sure where you're coming from, I want to make sure we're on the same page before I approve this PR for merging.

[anlutro]

I mean if we have this JSON data: [1,2,3] and we change it to [1,2,4] we could show that 3 has been removed and 4 has been added. But when I thought more about it, it would be really difficult to implement.

[terminalmage]

Oh OK, so if the data is being serialized and was formed as a list instead of a dict.

I guess you could always do type checking like you suggested, but we would still want there to be something in the changes key of the return dictionary if we were dealing with lists. the reason is that requisites such as onchanges look at the changes dict to detect whether or not any changes happened, so if we want these requisites to work properly, we do need to have some value there.

[isbm]

I have no much to add here what already has been said.

[cachedout]

@anlutro Are you ready for a final review here? I saw that you removed the WIP indicator from the PR title.

[anlutro]

Possibly - I wanted to see if any tests were failing first but seemed like Jenkins was freaking out.

…

On Tue, Aug 28, 2018, 20:03 Mike Place ***@***.***> wrote: @anlutro <https://github.com/anlutro> Are you ready for a final review here? I saw that you removed the WIP indicator from the PR title. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#48609 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJ9Fb078ADucwNSNgJwkephJQ61jCWkks5uVYYFgaJpZM4VRo2O> .

[rallytime]

@anlutro I think this test failure is related: https://jenkinsci.saltstack.com/job/pr-kitchen-ubuntu1604-py2/job/PR-48609/10/

Can you take a look?

[terminalmage]

There is still nothing in ret['changes'] if the data is non-dictionary. We need to have at least something in there for onchanges requisites to work.

[anlutro]

@rallytime Ah - fixed that now. Let's see if other failures pop up.

@terminalmage ret['changes'] will be set by file.check_managed_changes or file.manage_file, we just overwrite it if we think we can.

Sorry for the late replies, I'm out travelling.

[terminalmage]

Ah ok, thanks!

[rallytime]

@anlutro It's still failing, unfortunately:

https://jenkinsci.saltstack.com/job/pr-kitchen-ubuntu1604-py3/job/PR-48609/13/

[cachedout]

@anlutro Any updates here?

[anlutro]

Hey, sorry for going quiet. I see there's a test failure but it doesn't make any sense to me. It looks like self.run_state is returning a boolean - why? I'm pretty sure none of the code I added/modified could trigger this behavior.

[terminalmage]

I've started new tests since the old results are aged out of Jenkins. Pending getting them fixed, I think this is good to merge.

[cachedout]

@anlutro It looks like that test run failed. Can you take a look please? https://jenkinsci.saltstack.com/job/pr-kitchen-centos7-py3/job/PR-48609/40/testReport/junit/integration.states.test_file/FileTest/test_serializer_deserializer_opts/

[terminalmage]

I just pushed a commit to this PR to fix the docs build issue, @anlutro. You'll need to pull before you make any more changes.

[cachedout]

I'll rebase this PR in the mean time.

[terminalmage]

I found and fixed a problem in this PR, we were including the dictdiffer object in the changes dict and msgpack couldn't serialize it. 1af036a should fix this.