-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Remove test scope from bom #1557
Remove test scope from bom #1557
Conversation
This is related to quarkusio#1409. This commits remove the test scopes and add them when required in the different dependencies
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.
@cescoffier I added a few questions. Could you take a look?
</dependency> | ||
<dependency> | ||
<groupId>io.debezium</groupId> | ||
<artifactId>debezium-core</artifactId> | ||
<version>${debezium.version}</version> | ||
<type>test-jar</type> | ||
<scope>test</scope> |
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 remove the test
scope here but you don't set it in other parts of the code. Do we really use these dependencies? Do we want to set the versions?
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.
They are used in the integration tests. (integration-tests/main/pom.xml)
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 code was already using the test
scope.
@@ -536,7 +534,6 @@ | |||
<groupId>io.quarkus.gizmo</groupId> | |||
<artifactId>gizmo</artifactId> | |||
<type>test-jar</type> | |||
<scope>test</scope> | |||
<version>${gizmo.version}</version> |
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.
We don't use this dependency anywhere? Makes sense to have it anyway but we need to make sure we didn't miss a scope.
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.
It's used in core/deployment
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 code was already using the test
scope.
@@ -901,7 +891,6 @@ | |||
<groupId>org.apache.kafka</groupId> | |||
<artifactId>kafka_2.12</artifactId> | |||
<version>${kafka2.version}</version> | |||
<scope>test</scope> |
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.
Same question about the scope here. You don't set it in other parts of the code.
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.
Because the test
scope was already used.
@gsmet the dependencies are used but was already declaring the |
This fixes #1409.
This PR removes the test
scope
from the BOM and add them where it is required.Note that once merged a PR on the quickstarts will be required (quarkusio/quarkus-quickstarts#108).