Allow configuring working directory #1266
Conversation
jib-core/src/main/java/com/google/cloud/tools/jib/api/JibContainerBuilder.java
Outdated
Show resolved
Hide resolved
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ContainerParameters.java
Show resolved
Hide resolved
| .getWorkingDirectory() | ||
| .ifPresent( | ||
| workingDirectory -> | ||
| jibContainerBuilder.setWorkingDirectory(AbsoluteUnixPath.get(workingDirectory))); |
There was a problem hiding this comment.
We should probably be consistent as to the validation behavior of all the configurations that are AbsoluteUnixPath (workingDirectory, appRoot, volumes (#1278)) - currently for appRoot, an invalid absolute Unix-style path is thrown as AppRootInvalidException, but here it would be thrown as the IllegalArgumentException.
There was a problem hiding this comment.
Makes sense. Added WorkingDirectoryInvalidException, which follows the pattern of AppRootInvalidException. (However, I see InvalidContainerVolumeException in #1278 is a bit different.)
There was a problem hiding this comment.
Hmm, maybe the naming should start with Invalid... since in most exception names, the adjective comes first? (ie. IllegalArgumentException, NullPointerException, UnsupportedOperationException)
| @@ -146,7 +147,11 @@ public void buildDocker() | |||
|
|
|||
| } catch (AppRootInvalidException ex) { | |||
| throw new GradleException( | |||
| "container.appRoot is not an absolute Unix-style path: " + ex.getInvalidAppRoot()); | |||
| "container.appRoot is not an absolute Unix-style path: " + ex.getInvalidPathValue()); | |||
There was a problem hiding this comment.
Just noticed - should we include cause ex in these?
There was a problem hiding this comment.
Also, looked over AppRootInvalidException and it seems that in the constructor, message and invalidAppRoot are always set to the invalid appRoot.
There was a problem hiding this comment.
+1 to include cause ex.
It is actually intended that the message is set to appRoot (like FileNotFoundException, whose message is simply the missing file path).
There was a problem hiding this comment.
Yea, is the message parameter necessary then? I couldn't find a call to the constructor that wasn't appRoot repeated twice (new AppRootInvalidException(appRoot, appRoot, ex)).
There was a problem hiding this comment.
Not really necessary in our codebase, but I think it makes sense that, conceptually, the general exception message is set to appRoot rather than leaving it undefined. If anyone is doing a very general exception handling of displaying a message, it would tell the invalid value.
| .getWorkingDirectory() | ||
| .ifPresent( | ||
| workingDirectory -> | ||
| jibContainerBuilder.setWorkingDirectory(AbsoluteUnixPath.get(workingDirectory))); |
There was a problem hiding this comment.
Hmm, maybe the naming should start with Invalid... since in most exception names, the adjective comes first? (ie. IllegalArgumentException, NullPointerException, UnsupportedOperationException)
Fixes #1225.