[ RESENDING due to mail send failure yesterday ]
Hey Karen,
* Karen Tung (Karen.Tung at Sun.COM) wrote:
> Hi Glenn,
>
> Looks great overall, I have the following few comments.
>
> DC_defs.py: I don't think these 3 defined values are used anywhere, since
> the code to access the checksums are in a shell script. If they are not
> being
> used now, let's not define them until they need to be used.
Ok. Removed.
> generate_checksums:
>
> - Most of the shell scripts I read have the functions defined for
> the script right after any statically defined values. The compute_hash()
> function is kinda in the middle of the file, can you move it?
I wouldn't say it's in the middle. The bulk of the file prior to it's
location is comment/license text. I suppose I could move it above the
argument check. Would that be acceptable?
> - I think it is better to pass in $IMAGE and $HASH_TYPES as arguments
> to the compute_hash() function. That way, the function we are absolutely
> clear on exactly which value is being used, and it doesn't depend on things
> getting set correctly before the function is called.
I'll accept passing in $IMAGE since that changes but HASH_TYPES are
static in the script and I don't see any reason to 'pass' them in as
arguments. Unless I were to move the hash_type checking inside the
compute_hash() function and I don't think that buys us anything and it
would make the flow awkward (I don't believe checking to see if the
manifest included any hashes is best done inside compute_hash() which
means we need to set HASH_TYPES outside the function anyway). If
HASH_TYPES isn't set correctly (the only possible output is that it's
empty), we'll exit the script in the check specified hash types portion
before ever calling compute_hash().
> - Sometimes, you put {} around the variables, sometimes you don't, it is
> best to always put them in.
Accepted.
Thanks Karen!
--
Glenn
