-
Notifications
You must be signed in to change notification settings - Fork 1
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
build: Rework build and upgrade dependencies #3
Conversation
@@ -120,14 +215,6 @@ | |||
</execution> | |||
</executions> | |||
</plugin> | |||
<plugin> | |||
<artifactId>maven-surefire-plugin</artifactId> |
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 are we removing this?
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 declaration isn't needed, unit tests are run by default using this plugin.
docker run --rm -v $(pwd):/workspace -w /workspace maven:3-eclipse-temurin-17 mvn clean package
[INFO] Compiling 2 source files to /workspace/target/test-classes
[INFO]
[INFO] --- surefire:3.0.0:test (default-test) @ akash-java ---
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.
But this way we can define a version. I think we should keep it.
<version>${surefire.verion}</version> | ||
</plugin> | ||
<plugin> | ||
<artifactId>maven-failsafe-plugin</artifactId> |
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 are we removing failsafe?
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.
Failsafe is usually used to run integration tests (tests named after these patterns https://maven.apache.org/surefire/maven-failsafe-plugin/examples/inclusion-exclusion.html#inclusions).
This declaration isn't needed as long as there are no integration tests.
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.
fair enough
@@ -4,9 +4,9 @@ | |||
<pattern>%d{HH:mm:ss.SSS} [%thread] %-5level %logger{36} - %msg%n</pattern> | |||
</encoder> | |||
</appender> | |||
<logger name="cloud.quasarch" level="DEBUG" /> |
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.
Having this logger is handy for test debugging purposes, therefore i dont see why we should remove it
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 think it makes running tests a lot more verbose/noisy.
I can still restore it if you want to maintain this level.
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 guess it's good to have since this makes it clear for developers where and how can they tweak log levels for tests and that they can do it package level also.
@@ -120,14 +215,6 @@ | |||
</execution> | |||
</executions> | |||
</plugin> | |||
<plugin> | |||
<artifactId>maven-surefire-plugin</artifactId> |
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.
But this way we can define a version. I think we should keep it.
<version>${surefire.verion}</version> | ||
</plugin> | ||
<plugin> | ||
<artifactId>maven-failsafe-plugin</artifactId> |
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.
fair enough
@@ -4,9 +4,9 @@ | |||
<pattern>%d{HH:mm:ss.SSS} [%thread] %-5level %logger{36} - %msg%n</pattern> | |||
</encoder> | |||
</appender> | |||
<logger name="cloud.quasarch" level="DEBUG" /> |
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 guess it's good to have since this makes it clear for developers where and how can they tweak log levels for tests and that they can do it package level also.
I have submitted the requested changes. In addition, could we consider moving the examples module to its own project? Thanks |
Rework build and upgrade dependencies
Could we consider moving the
examples
module to its own project?