Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use AdoptOpenJDK image as default base image in Jib CLI Jar #3075

Merged
merged 12 commits into from
Feb 25, 2021

Conversation

mpeddada1
Copy link
Contributor

No description provided.

@mpeddada1 mpeddada1 requested a review from a team February 24, 2021 15:43
@google-cla google-cla bot added the cla: yes label Feb 24, 2021
@codecov
Copy link

codecov bot commented Feb 24, 2021

Codecov Report

Merging #3075 (9c99e86) into master (fd8aed6) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3075      +/-   ##
============================================
+ Coverage     71.15%   71.19%   +0.03%     
- Complexity     2307     2312       +5     
============================================
  Files           278      278              
  Lines          9746     9759      +13     
  Branches        989      990       +1     
============================================
+ Hits           6935     6948      +13     
  Misses         2471     2471              
  Partials        340      340              
Impacted Files Coverage Δ Complexity Δ
...a/com/google/cloud/tools/jib/cli/jar/JarFiles.java 96.00% <100.00%> (+0.54%) 4.00 <4.00> (+1.00)
.../google/cloud/tools/jib/cli/jar/JarProcessors.java 87.17% <100.00%> (+0.69%) 13.00 <6.00> (ø)
...tools/jib/cli/jar/SpringBootExplodedProcessor.java 96.42% <100.00%> (+0.08%) 23.00 <2.00> (+1.00)
...tools/jib/cli/jar/SpringBootPackagedProcessor.java 100.00% <100.00%> (ø) 4.00 <2.00> (+1.00)
...d/tools/jib/cli/jar/StandardExplodedProcessor.java 94.59% <100.00%> (+0.30%) 8.00 <2.00> (+1.00)
...d/tools/jib/cli/jar/StandardPackagedProcessor.java 100.00% <100.00%> (ø) 5.00 <2.00> (+1.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd8aed6...9c99e86. Read the comment docs.

Copy link
Member

@chanseokoh chanseokoh left a comment

Choose a reason for hiding this comment

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

The plan is to check the Java version and choose between Java 8 and 11, right?

chanseokoh
chanseokoh previously approved these changes Feb 24, 2021
@@ -198,8 +198,9 @@ public Integer call() {
CacheDirectories cacheDirectories =
CacheDirectories.from(commonCliOptions, jarFile.toAbsolutePath().getParent());
JarProcessor processor = JarProcessors.from(jarFile, cacheDirectories, mode);
Integer jarJavaVersion = JarProcessors.getJavaMajorVersion(jarFile);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of making getJavaMajorVersion() public and calling it twice (although I have no idea if there will be noticeable performance impact when "opening a jar" twice with new JarFile() in this case), how about adding a field JarProcessor.javaVersion and a getter for it? (Maybe the current getJavaMajorVersion() should be renamed to, say, {read|determine}JavaMajorVersion if we do so.) That way, we can avoid running the code twice. I see JarProcessor is already passed into toJibContainerBuilder(), so it just needs to call the getter.

Copy link
Contributor Author

@mpeddada1 mpeddada1 Feb 24, 2021

Choose a reason for hiding this comment

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

Ah I missed this. Thanks for pointing this out - adding a getter to JarProcessor is a good idea. Since interfaces don't allow instance fields, I had to add a parameter to the constructors of the JarProcessor instances which resulted in a couple of extra changes to the tests.

@chanseokoh chanseokoh dismissed their stale review February 24, 2021 19:00

diversion from the original plan

Copy link
Member

@chanseokoh chanseokoh left a comment

Choose a reason for hiding this comment

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

LGTM, only minor comments.

In many cases, the previous approach to call some methods multiple times is better for readability and compact code in general. It's just that this was a special case involving opening a jar. Although it can still be the case that opening and inspecting is really quick and all of this is meaningless. But yeah, I think the current pattern doesn't look bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants