Skip to content
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

Expose full SHA256 hash in file name placeholders #2770

Closed
wants to merge 1 commit into from

Conversation

mislav
Copy link
Contributor

@mislav mislav commented Mar 22, 2019

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

Description

Currently, [hash] is truncated to 8 characters, so add another placeholder [fullhash] to gain access to the untruncated value. The problem with truncated values is that the risk of collisions is larger.

Ideally, [hash] would be the full value itself and users could be given some other mechanism of truncating the value to a specific number of characters (e.g. [hash:8]), but changing that at this point would break backwards compatibility.

This is WIP and soliciting feedback! Open questions:

  • Is [fullhash] an OK placeholder name?
  • Where should I add tests for this? It's not clear to me where [hash] is tested in the first place; many different tests break if I purposely change [hash] behavior.

Thank you!

/cc @keithamus

Currently, `[hash]` is truncated to 8 characters, so add another
placeholder `[fullhash]` to gain access to the untruncated value.
@lukastaegert
Copy link
Member

I still wonder how many billions of builds you deploy each day to get a reasonable risk of a hash collision. Previous hashing issues had nothing to do with this. 16^8 = 4.294.967.296
But of course one can always talk about these things

@mislav
Copy link
Contributor Author

mislav commented Mar 22, 2019

@lukastaegert I agree that the risk of collision is relatively small; but I find it odd that the full checksum is calculated but never publicly accessible.

Yes, I understand that previous hashing issues were due to ordering and race conditions and not due to collisions.

Alternatively, the full checksum could be available to a custom function if #2585 is implemented.

@lukastaegert
Copy link
Member

Alternatively, the full checksum could be available to a custom function if #2585 is implemented.

This would of course be the best solution but for the time being, providing [fullhash] as an additional option is ok with me. If you promise to actually use it!

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to merge this, we need a test that uses the new name pattern element.

@lukastaegert
Copy link
Member

Also, it needs to be documented in the "big list of options" in the docs folder.

@sormy
Copy link
Contributor

sormy commented Apr 28, 2019

Probably [hash:X] could be a better option, where X is a number of characters in the hash

@shellscape
Copy link
Contributor

@mislav Would you be up for adding the tests requested? It also looks like your fork could use a rebase and a few conflicts need to be addressed.

@mislav
Copy link
Contributor Author

mislav commented Aug 19, 2019

I haven't had time to rebase or to figure out where or how to add the new tests. Other people are welcome to use my branch as the starting point of implementing the same or similar feature. 🙇

@mislav mislav closed this Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants