GoogleContainerTools / container-debug-support

Language-runtime support files for in-container debugging
Apache License 2.0
93 stars 25 forks source link

Adds JibAdapter and JibMavenPluginAdapter. #19

Closed coollog closed 5 years ago

coollog commented 6 years ago

Part of #13

coollog commented 6 years ago

I was thinking of handling isVersionSupported==false with giving a warning to users about the version of Jib being unsupported. For example, if jib-maven-plugin was version 0.10.0, skaffold-maven-plugin that only supports up to 0.9.x would warn that the 0.10.0 version was not supported, but still try to use the adapter for the 0.9.x versions to find to.image. This will allow for new releases of jib-maven-plugin to not immediately break skaffold-maven-plugin that has not been updated yet.

For Jib not found, I'm trying to keep this codebase lighter on exceptions, since those added a lot of catches and throws everywhere in the Jib codebase - and only have exceptions for cases that are truly exceptions and not something that is expected (like not having jib-maven-plugin). Giving back a ResolvedJib will allow the caller to decide how to handle the resolution result without using messy try-catch blocks. Does this sound reasonable?

coollog commented 6 years ago

Example use:

ResolvedJib resolution = JibMavenPluginAdapter.resolve(mavenProject);
if (!resolution.hasJib()) {
  return tryOtherWaysToGetTargetImageReference();
}
if (!resolution.isSupportedVersion()) {
  warn("Jib version is not supported");
}
if (!resolution.getImageReference.isPresent()) {
  warn("Jib present but target image reference not defined");
  return tryOtherWaysToGetTargetImageReference();
}
return resolution.getImageReference().get();
briandealwis commented 6 years ago

That makes sense. I think my confusion comes from the use of Resolved in ResolvedJib which suggests that the resolution was successful. Would ResolvedJibJibParameters work?