Add support for sha1 step to hash files #32
Conversation
|
I guess this plugins is maintained by Cloudbees employees so I imagine it must be reviewed by them as well. So... cc @reviewbybees |
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
| return filePath.act(new ComputeSha1()); | ||
| } | ||
|
|
||
| private class ComputeSha1 implements FilePath.FileCallable<String> { |
There was a problem hiding this comment.
Should use MasterToSlaveFileCallable instead.
| private class ComputeSha1 implements FilePath.FileCallable<String> { | ||
| @Override | ||
| public String invoke(File file, VirtualChannel virtualChannel) throws IOException, InterruptedException { | ||
| HashFunction hf = Hashing.sha1(); |
There was a problem hiding this comment.
I get a bit nervous when I see google libraries used in Jenkins code :) back-compat issues, remoting problems and what not that has been happening in the past.
Might not be an issue here, but isn't there a perfectly good api for this in the standard library?
| @Override | ||
| public String invoke(File file, VirtualChannel virtualChannel) throws IOException, InterruptedException { | ||
| HashFunction hf = Hashing.sha1(); | ||
| HashCode hc = hf.newHasher().putBytes(Files.toByteArray(file)).hash(); |
There was a problem hiding this comment.
You should check that the file exists and is a file before trying to read it and provide a usefull error message if it isn't.
| /* | ||
| * The MIT License (MIT) | ||
| * | ||
| * Copyright (c) 2017 CloudBees Inc. |
There was a problem hiding this comment.
Did I miss a mail welcoming you to CloudBees :D if not then you can't claim copyright for anyone but yourself or the entity you are doing the work for ;)
| /** | ||
| * Tests {@link TouchStep}. | ||
| * | ||
| * @author Robert Sandell <rsandell@cloudbees.com>. |
| return null; | ||
| } | ||
| } catch (NoSuchAlgorithmException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
This will just end up in the agent log where no one will find it. Probably better to just let the exception pass through all the way up to run since it should happen very rarely.
There was a problem hiding this comment.
The signature of invoke only allows IOException and InterruptedException. Should I wrap NoSuchAlgorithmException with one of those or into a runtime exception?
There was a problem hiding this comment.
Yes, wrapping it in an IOException would be acceptable I guess :)
|
Better? |
| try { | ||
| return sha1(file); | ||
| } catch (NoSuchAlgorithmException e) { | ||
| throw new IOException(e.getMessage()); |
There was a problem hiding this comment.
well it's cyustomary to provide the original exception as the cause of the new one, but I'll allow it :)
|
Thanks! |
This can be useful under many circumstances, in particular when building docker images containing some kind of cache for a package manager.