Salt: Should file.patch state require a hash?

[MikeH-Halo]

Description of Issue/Question

The file.patch state requires a hash today. In my case I have patch file templates which are applied using certain criteria (grains, nodegroups, what have you). The hash is impossible to calculate beforehand, and I am currently setting the hash to zero just get it to run (even if the state fails).

My suggestion is that if the hash is omitted then file.patch should succeed if the underlying patch command succeeds.

Setup

(Please provide relevant configs and/or SLS files (Be sure to remove sensitive info).)

Steps to Reproduce Issue

(Include debug logs if possible and relevant.)

Versions Report

(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)

[gtmanfred]

@terminalmage do you have any opinions about this?

I am pretty sure that file.patch can be set to verify=False, and the default behavior of all the other file states is to have a source_hash.

Also, most patches IMHO are going to be pulled down and just need to be verified, and not generated from templates, so they should have hash sums.

I disagree with the statement that just because it succeeds to be applied it shouldn't need to have a source hash. We could make the same argument about package managers, just because they have to do more work, and get the source hash into the database file in the repository, doesn't mean we shouldn't still have gpg signatures.

Thanks,
Daniel

Edit: Ignore this.

[MikeH-Halo]

Just to be clear; I never made any statements that the hash isn't necessary. What I said was; if the hash _isn't provided_ then file.patch should succeed if the underlying patch command succeeds. That is something different.
However, if I can set verify=False then that suffices for me..

[terminalmage]

@gtmanfred it looks like flie.patch has no verify argument, in fact the state will refuse to run if there is no hash.

[gtmanfred]

Ahh, my mistake.

I thought file.patch has a skip_verify options, but it does not appear that it does. We should totally add one.

Thanks,
Daniel

[terminalmage]

The proper fix here, I think, would be to normalize the arguments in the file.patch state to make them consistent with file.managed. That is, we would rename hash to source_hash (and deprecate hash), and add a skip_verify. We would also not require hashes for local files (or those served up via the Salt fileserver, i.e. those with salt:// URIs).

@gtmanfred, @MikeH-Halo thoughts?

[gtmanfred]

:+1: from me

[terminalmage]

:tada: :balloon: :tada:

I shall make it so, after I'm done with my PY3 compatibility work.

[MikeH-Halo]

On my wishlist is also templating ability so I can generate my patches from a template in one go.
Now I need to file.manage the patch template first to a local temporary file, and then file.patch it. Maybe not the cleanest way to get things done.. :-)
Otherwise: 👍 from me too..
Thanks!

[terminalmage]

@MikeH-Halo adding template support should also be easy, we can simply invoke the file.managed state from with the file.patch state to pull down the patch file, and we'll get all the templating, hash verification, etc. support for free.

[disaster123]

file.patch also states to support directorys? "name The file or directory to which the patch will be applied." How should i derive a hash of a directory?

[gtmanfred]

The hash is for the source, the patch, not the name.
On Thu, Apr 20, 2017 at 6:43 AM disaster123 [e-mail]
wrote:

file.patch also states to support directorys? "name The file or directory
to which the patch will be applied." How should i derive a hash of a
directory?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/saltstack/salt/issues/40146#issuecomment-295723130,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAssofcPtSlERS1t7M1uyQvLF0Jfmg4Fks5rx1LmgaJpZM4MiJkg
.

[disaster123]

sure? I tried to use file.patch today and it failed if name is a directory cause it tries to calculate a hash from the dir

[gtmanfred]

Can you open another issue about this and provide a log of the error and an easy way to reproduce the error?

Thanks,
Daniel

[MikeH-Halo]

Commenting here for future reference: workaround setting the hash to 0 (zero) turns off hash-checking. (at least for 2017.7.2)
Maybe documentation should reflect this?

[PhilippeAB]

@MikeH-Halo thanks for the tip you juste save my day
@gtmanfred according to the doc the hash is on the file to be patched to avoid patching allready patched files

[terminalmage]

This state has been rewritten in https://github.com/saltstack/salt/pull/47010 for the next feature release. It scraps completely the hash argument, using a completely different method to check if the patch would be cleanly applied before we even attempt to make any changes. It also expands the patch sources to include any remote source (http(s), ftp, etc.), and adds support for templating the patch file.

[MikeH-Halo]

Awesome!