-
Notifications
You must be signed in to change notification settings - Fork 480
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
Log Docker Build Context Directory while building image in Simple Dockerfile mode #1192
Log Docker Build Context Directory while building image in Simple Dockerfile mode #1192
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1192 +/- ##
=========================================
Coverage 50.15% 50.16%
- Complexity 3698 3699 +1
=========================================
Files 457 457
Lines 20605 20607 +2
Branches 2805 2805
=========================================
+ Hits 10334 10337 +3
+ Misses 9187 9185 -2
- Partials 1084 1085 +1
Continue to review full report at Codecov.
|
@@ -181,7 +182,7 @@ private static void verifyImageNames(List<ImageConfiguration> ret) { | |||
} | |||
} | |||
|
|||
public static List<ImageConfiguration> initImageConfiguration(String apiVersion, Date buildTimeStamp, JavaProject javaProject, List<ImageConfiguration> images, ImageConfigResolver imageConfigResolver, KitLogger log, String filter, ConfigHelper.Customizer customizer) { | |||
public static List<ImageConfiguration> initImageConfiguration(String apiVersion, Date buildTimeStamp, JavaProject javaProject, List<ImageConfiguration> images, ImageConfigResolver imageConfigResolver, KitLogger log, String filter, ConfigHelper.Customizer customizer, JKubeConfiguration jKubeConfiguration) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't checked the rest, but JKubeConfiguration should already contain the JavaProject instance (So replacing one with the other should do in this case).
@@ -211,6 +212,7 @@ private static void verifyImageNames(List<ImageConfiguration> ret) { | |||
BuildConfiguration buildConfiguration = image.getBuildConfiguration(); | |||
if (buildConfiguration != null && buildConfiguration.isDockerFileMode()) { | |||
log.info("Using Dockerfile: %s", buildConfiguration.getDockerFile().getAbsolutePath()); | |||
log.info("Using Docker Context Directory: %s", buildConfiguration.getAbsoluteContextDirPath(jKubeConfiguration.getSourceDirectory(), jKubeConfiguration.getBasedir().getAbsolutePath())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't these directories in JavaProject?
I remember in the early days these fields were scattered all around and we managed to reduce them to just the JKubeConfiguration and the JavaProject instances.
What's the issue in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the homonymous fields have the same purpose, one set should be deprecated. If not, the undocumented, should be. This way we would avoid situations like the current were there's confusion about what should be used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see jKubeConfiguration.getBaseDir()
returns JavaProject's base directory. But I'm not able to find any sourceDirectory
equivalent in JavaProject
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log.info("Using Docker Context Directory: %s", buildConfiguration.getAbsoluteContextDirPath(jKubeConfiguration.getSourceDirectory(), jKubeConfiguration.getBasedir().getAbsolutePath())); | |
log.info("Using Docker context directory: %s", buildConfiguration.getAbsoluteContextDirPath(jKubeConfiguration.getSourceDirectory(), jKubeConfiguration.getBasedir().getAbsolutePath())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see jKubeConfiguration.getBaseDir() returns JavaProject's base directory. But I'm not able to find any sourceDirectory equivalent in JavaProject
OK, so this one is not duplicate, it just retrieves javaProject.baseDirectory
However, outputDirectory
is still defined in both places. We'll need to investigate and deprecate, rename or proper document them.
929fb20
to
91e7aa7
Compare
|
||
public class ConfigHelperTest { | ||
@Mocked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using Mockito annotations ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'd need to run the test with MockitoJUnitRunner.class
These annotations were the ones provided by JMockit.
private ImageConfigResolver mockedImageConfigResolver; | ||
|
||
private OpenshiftResourceMojo openShiftResourceMojo; | ||
|
||
@Before | ||
public void setUp() throws IOException { | ||
public void setUp() { | ||
mockedClusterAccess = mock(ClusterAccess.class, RETURNS_DEEP_STUBS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like there are too many nested mocks here which may not be needed if you use RETURNS_DEEP_STUBS
and or @InjectMocks
. It makes this part hard to read (and maintain).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, you're right. I was able to remove unnecessary mocks for JKubeConfiguration class by using RETURNS_DEEP_STUBS
@Before | ||
public void setUp() { | ||
imageConfigResolver = mock(ImageConfigResolver.class, RETURNS_DEEP_STUBS); | ||
logger = mock(KitLogger.class, RETURNS_DEEP_STUBS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are not verifying any of the logged messages, SilentLogger
is a good option for tests instead of using a mock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have some examples of using it? I'm not able to find any references to SilentLogger
in this repository
b52c43d
to
d40505e
Compare
@Before | ||
public void setUp() { | ||
imageConfigResolver = mock(ImageConfigResolver.class, RETURNS_DEEP_STUBS); | ||
logger = mock(KitLogger.SilentLogger.class, RETURNS_DEEP_STUBS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is not to mock the logger unless we need to verify something.
logger = mock(KitLogger.SilentLogger.class, RETURNS_DEEP_STUBS); | |
logger = new KitLogger.SilentLogger(); |
d40505e
to
0509f31
Compare
...aven-plugin/plugin/src/main/java/org/eclipse/jkube/maven/plugin/mojo/build/ResourceMojo.java
Outdated
Show resolved
Hide resolved
…kerfile mode Add a log statement for Docker Build context directory when building a docker image in Simple Dockerfile mode. Related to eclipse-jkube#1055 Signed-off-by: Rohan Kumar <rohaan@redhat.com>
0509f31
to
262b449
Compare
SonarCloud Quality Gate failed. |
Description
Add a log statement for Docker Build context directory when building a
docker image in Simple Dockerfile mode.
Related to #1055
Signed-off-by: Rohan Kumar rohaan@redhat.com
Type of change
test, version modification, documentation, etc.)
Checklist