Conversation
ferki
left a comment
ferki
left a comment
There was a problem hiding this comment.
Thanks for the contribution and sorry about the late feedback!
It seems like this PR tries to address #485 by also mixing in parts of #721. That feels like trying to do a bit too much in a single step (adding a new test and more digest algorithms), while leaving some parts of the story out (the new digest algorithms now only available within Rex::Test instead of being general commands).
Maybe it would be better to just do these step-by-step like:
- add a checksum test to Rex::Test based on what's already available in core (md5)
- add support for more digest algorithms (perhaps in a similar way as seen here, and/or by also introducing a new module like
Rex::Commands::Digest) - extend
has_checksumin Rex::Test to support all the new digest algorithms
I'll keep it open and unmerged for now, as it might be possible to cherry-pick parts of the PR later for the reduced "MD5-only" scope.
| @@ -0,0 +1,54 @@ | |||
| # | |||
| # (c) Jan Gehring <jan.gehring@gmail.com> | |||
There was a problem hiding this comment.
IANAL, but I'm pretty sure in this case the copyright is being held by the author of this file (and not by Jan :)).
| } | ||
|
|
||
| sub run_test { | ||
| my ( $self, $file, $checksum, $algo, $computed ) = (shift, shift, shift, shift); |
There was a problem hiding this comment.
I'm not entirely sure about using positional arguments here. Perhaps it might be better with a hash?
Also, there are 5 variables on the left hand side, but only 4 shifts on the right hand side. Perhaps $computed can be dropped since doesn't seem like used as an input argument?
| @@ -0,0 +1,54 @@ | |||
| use strict; | |||
3 participants
Aim
Fix #485
Only supporting three hash algorithms, currently md5, sha1 and sha256, the former using existing code (
md5fromRex::Commands::MD5), the latter two usingDigest::SHA.