Deprecate jib.extraDirectory in Gradle #1671
Conversation
|
Did a lot of manual testing and fixed a few things. Really ready for review. |
| // non-plural to retain backward-compatibility for the "jib.extraDirectory.path" config parameter | ||
| public void setPath(Object paths) { | ||
| public void setPaths(Object paths) { | ||
| jibExtension.extraDirectoriesConfigured = true; |
There was a problem hiding this comment.
I'm curious how necessary this is, would prefer we didn't need to keep track of things with flags?
There was a problem hiding this comment.
Yeah, this is quite unfortunate. The Gradle case wasn't easy for a few reasons.
- I couldn't find a way (if at all) to log inside
JibExtensionorExtraDirectoriesParameters. - The way we can extract
extraDirectories.pathsorextraDirectory.pathin our codebase for consumption is to first go throughgetExtraDirectories(), which is also used by Gradle to set in config values, so unlike Maven, it's not possible to check if the user is using the old or new syntax (or both) insidegetExtraDirectories(). On top of that, Gradle always calls bothgetExtraDirectory()andgetExtraDirectories()regardless of whether the user has ever configured anything.
So, although this is very unfortunate, I couldn't find a good alternative.
| // non-plural to retain backward-compatibility for the "jib.extraDirectory" config parameter | ||
| public void setExtraDirectory(Object extraDirectories) { | ||
| this.extraDirectory.setPath(extraDirectories); | ||
| public void setExtraDirectories(Object extraDirectories) { |
There was a problem hiding this comment.
Wouldn't we want to avoid adding getters and setters for extraDirectories? I thought in maven we're pushing everyone to use the complex type configuration for <extraDirectories><paths..... I would image we would want to do that here as well... and just not allow jib.extraDirectories = asdf
There was a problem hiding this comment.
Sounds good with being consistent. We allowed the shortcut to remain backward compatibility after introducing .permissions, but we can clean that up since we are making breaking changes with deprecation.
jib-maven-plugin/src/test/resources/maven/projects/simple/pom-complex-properties.xml
Show resolved
Hide resolved
jib-gradle-plugin/src/integration-test/resources/gradle/projects/simple/build-extra-dirs.gradle
Outdated
Show resolved
Hide resolved
|
Good for another look again. |
...rc/integration-test/java/com/google/cloud/tools/jib/gradle/SingleProjectIntegrationTest.java
Outdated
Show resolved
Hide resolved
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/TaskCommon.java
Show resolved
Hide resolved
...rc/integration-test/java/com/google/cloud/tools/jib/gradle/SingleProjectIntegrationTest.java
Outdated
Show resolved
Hide resolved
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/TaskCommon.java
Outdated
Show resolved
Hide resolved
The last PR for #1020.
Good for review, but I haven't done a lot of manual testing, so I'll give a second pass myself too.