Skip to content
This repository was archived by the owner on Sep 12, 2024. It is now read-only.

Fixed check for volume source being null or empty #1044

Closed
wants to merge 4 commits into from
Closed

Fixed check for volume source being null or empty #1044

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3eaeaf9
Fixed check for source being null or empty
piccobit Dec 15, 2016
6f47ce9
Merge pull request #1 from piccobit/fix-job-volume-validation
piccobit Dec 15, 2016
c6700b4
Separate checks if volume source is valid
piccobit Dec 15, 2016
e15c500
Merge pull request #2 from piccobit/fix-job-validator-separate-checks-1
piccobit Dec 15, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion helios-client/src/main/java/com/spotify/helios/common/JobValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,11 @@ public Set<String> validate(final Job job) {
errors.add("Volume path is not absolute: " + path);
continue;
}
if (!isNullOrEmpty(source) && !source.startsWith("/")) {
if (isNullOrEmpty(source)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can just change && to ||, but I guess more specific errors can't hurt.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@piccobit thanks - it would be helpful to see both changes in context.

errors.add("Volume source is null or empty");
continue;
}
if (!source.startsWith("/")) {
errors.add("Volume source is not absolute: " + source);
continue;
}
Expand Down