-
Notifications
You must be signed in to change notification settings - Fork 3
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
Chore/dev updates #46
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the only "real" code change in this PR--switching from secure_compare mix package to plug_crypto
bc88a96
to
dadbaa5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really simple! Seems good.
@header_key :"X-APIAuth-Content-Hash" | ||
@md5_header_key :"Content-MD5" | ||
@value_key :content_hash | ||
@value_key :content_hash | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirming, because a change in the header type broke us in the past: this is just a formatting change, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup! just an automated spacing change
def compare(hc, keys) do | ||
compare(hc, keys, &SecureCompare.compare/2) | ||
compare(hc, keys, &Crypto.secure_compare/2) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding here: where previously, we were passing in a pointer to &SecureCompare.compare, we've replaced that with &Crypto.secure_compare.
The new lib doesn't require any kind of config args?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://elixir-lang.org/getting-started/modules-and-functions.html#function-capturing < here's some more info on the & /2
syntax. both methods have arity of 2 and accept 2 binaries no changes should be needed (but we'll make sure this still works on non-prod)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Late to the party, but 🛳️
Description of changes