Fixed check for volume source being null or empty #1044
Conversation
This bug was hidden by the second condition which is normally always false and was only discovered when I disabled this second condition to allow the usage of named volumes which don't start with a '/'.
Fixed check for source being null or empty
| @@ -150,7 +150,7 @@ public JobValidator(final boolean shouldValidateJobHash, | |||
| errors.add("Volume path is not absolute: " + path); | |||
| continue; | |||
| } | |||
| if (!isNullOrEmpty(source) && !source.startsWith("/")) { | |||
| if (isNullOrEmpty(source) && !source.startsWith("/")) { | |||
There was a problem hiding this comment.
Are you sure this logic wasn't correct before? Wouldn't this now throw an NPE if source is null?
There was a problem hiding this comment.
@rohansingh Ah, yes, you're right! In our use case with the named volume I've removed the second condition completely and also the printing of the source in case of an error!
There was a problem hiding this comment.
Ok, I've separated the checks for the volume source to prevent the NPE and requested another PR.
Apologize the mess, these are my first PRs, I'm more a Gerrit guy. ;-)
Separate checks if volume source is valid
| @@ -150,7 +150,11 @@ public JobValidator(final boolean shouldValidateJobHash, | |||
| errors.add("Volume path is not absolute: " + path); | |||
| continue; | |||
| } | |||
| if (!isNullOrEmpty(source) && !source.startsWith("/")) { | |||
There was a problem hiding this comment.
We can just change && to ||, but I guess more specific errors can't hurt.
| @@ -150,7 +150,11 @@ public JobValidator(final boolean shouldValidateJobHash, | |||
| errors.add("Volume path is not absolute: " + path); | |||
| continue; | |||
| } | |||
| if (!isNullOrEmpty(source) && !source.startsWith("/")) { | |||
| if (isNullOrEmpty(source)) { | |||
There was a problem hiding this comment.
it seems like before, a null or empty source was allowed. Now this will cause an error. Am I correct on that and is that change intentional?
There was a problem hiding this comment.
@mattnworb I'm preparing another PR adding an option to allow also 'named volumes' and this PR needs the above change.
It would be fine for me if you cancel this PR and my new PR includes the changes above.
There was a problem hiding this comment.
@piccobit thanks - it would be helpful to see both changes in context.
|
we seem to mention in https://github.com/spotify/helios/blob/master/docs/user_manual.md#volumes that the right-hand side of the volume can be left off or empty |
|
Ah, the original code makes sense then. |
This bug was hidden by the second condition which is normally always false and was only discovered when I disabled the second condition to allow the usage of named volumes which don't start with a '/'.