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

file.managed: Allow local file sources to use source_hash #29289

Merged
merged 1 commit into from Dec 1, 2015

Conversation

Projects
None yet
3 participants
@terminalmage
Member

terminalmage commented Nov 30, 2015

Fixes #26526.

@terminalmage

This comment has been minimized.

Show comment
Hide comment
@terminalmage

terminalmage Nov 30, 2015

Member

While the diff may not be 100% clear on this, all this PR does is move the check if the file is a local file below where source_hash is evaluated, so that using a sha256 hash is done as a fallback if source_hash is not provided.

Member

terminalmage commented Nov 30, 2015

While the diff may not be 100% clear on this, all this PR does is move the check if the file is a local file below where source_hash is evaluated, so that using a sha256 hash is done as a fallback if source_hash is not provided.

source_sum = {'hsum': file_sum, 'hash_type': 'sha256'}
elif source.startswith('/'):
file_sum = get_hash(source, form='sha256')
source_sum = {'hsum': file_sum, 'hash_type': 'sha256'}
elif source_hash:
protos = ('salt', 'http', 'https', 'ftp', 'swift', 's3')

This comment has been minimized.

@lorengordon

lorengordon Dec 1, 2015

Contributor

@terminalmage, can we add 'file' to this list of accepted source_hash protos? Will that allow us to specify a hash in the form: source_hash: file:///path/to/hashfile.hash?

@lorengordon

lorengordon Dec 1, 2015

Contributor

@terminalmage, can we add 'file' to this list of accepted source_hash protos? Will that allow us to specify a hash in the form: source_hash: file:///path/to/hashfile.hash?

This comment has been minimized.

@terminalmage

terminalmage Dec 1, 2015

Member

Yeah, that would work. If you want to submit the PR, go ahead, otherwise I can do it.

@terminalmage

terminalmage Dec 1, 2015

Member

Yeah, that would work. If you want to submit the PR, go ahead, otherwise I can do it.

This comment has been minimized.

@lorengordon

lorengordon Dec 1, 2015

Contributor

Alright, I'm on it.

@lorengordon

lorengordon Dec 1, 2015

Contributor

Alright, I'm on it.

@cachedout

This comment has been minimized.

Show comment
Hide comment
@cachedout

cachedout Dec 1, 2015

Contributor

Makes sense. Cool.

Contributor

cachedout commented Dec 1, 2015

Makes sense. Cool.

cachedout added a commit that referenced this pull request Dec 1, 2015

Merge pull request #29289 from terminalmage/issue26526
file.managed: Allow local file sources to use source_hash

@cachedout cachedout merged commit 0fd3e8b into saltstack:2015.8 Dec 1, 2015

4 of 6 checks passed

jenkins/salt-pr-rs-cent6-n Salt PR - RS CentOS 6 #299 — FAILURE
Details
default Merged build started.
Details
jenkins/salt-pr-clone Salt PR - Clone Repository #11589 — SUCCESS
Details
jenkins/salt-pr-linode-ubuntu14.04-n Salt PR - Linode Ubuntu 14.04 #2656 — SUCCESS
Details
jenkins/salt-pr-lint-n Salt PR - Code Lint #11297 — SUCCESS
Details
jenkins/salt-pr-rs-cent7-n Salt PR - RS CentOS 7 #10124 — SUCCESS
Details
@lorengordon

This comment has been minimized.

Show comment
Hide comment
@lorengordon

lorengordon Dec 1, 2015

Contributor

Maybe a slight tangent, but I think this mesage is not quite right when the source_hash scheme matches the protos list. The message seems to be saying the syntax is something like source_hash: md5=salt://path/to/hashfile.md5. But the code passes source_hash without modification directly to _urlparse(), so I think the message is wrong. The syntax would just be source_hash: <proto>://path/to/hashfile. Perhaps the message is intending to explain how extract_hash() reads the file, but I don't think that is coming across. I think that is covered in the file state docs. Also, it would maybe be nice if the message referenced the allowed protos.

Contributor

lorengordon commented Dec 1, 2015

Maybe a slight tangent, but I think this mesage is not quite right when the source_hash scheme matches the protos list. The message seems to be saying the syntax is something like source_hash: md5=salt://path/to/hashfile.md5. But the code passes source_hash without modification directly to _urlparse(), so I think the message is wrong. The syntax would just be source_hash: <proto>://path/to/hashfile. Perhaps the message is intending to explain how extract_hash() reads the file, but I don't think that is coming across. I think that is covered in the file state docs. Also, it would maybe be nice if the message referenced the allowed protos.

@lorengordon

This comment has been minimized.

Show comment
Hide comment
@lorengordon

lorengordon Dec 1, 2015

Contributor

Oops, already merged. Shucks.

Contributor

lorengordon commented Dec 1, 2015

Oops, already merged. Shucks.

@cachedout

This comment has been minimized.

Show comment
Hide comment
@cachedout

cachedout Dec 1, 2015

Contributor

@lorengordon Can you send over a PR with those changes? I'd love to include them.

Contributor

cachedout commented Dec 1, 2015

@lorengordon Can you send over a PR with those changes? I'd love to include them.

@lorengordon

This comment has been minimized.

Show comment
Hide comment
@lorengordon

lorengordon Dec 1, 2015

Contributor

@cachedout, yep, see #29305.

Contributor

lorengordon commented Dec 1, 2015

@cachedout, yep, see #29305.

@cachedout

This comment has been minimized.

Show comment
Hide comment
@cachedout

cachedout Dec 1, 2015

Contributor

@lorengordon Thank you very much.

Contributor

cachedout commented Dec 1, 2015

@lorengordon Thank you very much.

@terminalmage

This comment has been minimized.

Show comment
Hide comment
@terminalmage
Member

terminalmage commented Dec 1, 2015

@lorengordon Thanks!

@terminalmage terminalmage deleted the terminalmage:issue26526 branch Jan 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment