-
Notifications
You must be signed in to change notification settings - Fork 23
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
Feature/gradle x #314
Feature/gradle x #314
Conversation
I'll take a look at this on Monday. |
0a285c1
to
fd396ae
Compare
sample-apps/liberty-app/build.gradle
Outdated
testCompile group: 'org.slf4j', name: 'slf4j-log4j12', version: '1.7.36' | ||
testImplementation 'org.testcontainers:junit-jupiter:1.19.1' | ||
testImplementation project(':microshed-testing-liberty') | ||
testImplementation project(':microshed-testing-testcontainers') |
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.
Liberty includes microshed-testing-testcontainers
no need to include it here.
testImplementation project(':microshed-testing-testcontainers') |
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 thought so too but since change some gradle dependencies from compile to implementation, the behavior is different. That's why I need your help, I'm not gr8 at gradle. Without this explicit dependency in this sample app, it will not compile. This leads me to believe that the dependency: testcontainers is no longer part of the artifact created in the liberty module.
testCompile group: 'org.slf4j', name: 'slf4j-log4j12', version: '1.7.36' | ||
testImplementation 'org.testcontainers:junit-jupiter:1.19.1' | ||
testImplementation project(':microshed-testing-payara-server') | ||
testImplementation project(':microshed-testing-testcontainers') |
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.
Payara includes microshed-testing-testcontainers no need to include it here.
testImplementation project(':microshed-testing-testcontainers') |
// TODO solve why needed also regular core to compile? | ||
testImplementation project(':microshed-testing-core') |
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.
This shouldn't be necessary. I can do some testing to figure out why this working working.
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.
Again this points out my issue as described on your comment in Liberty sample app. I think we should use some other dependecy scope in the core and the modules to resolve this issues.
mongo.start(); | ||
mockServer.start(); | ||
app.start(); | ||
// by default evertything will start in parallel | ||
SharedContainerConfiguration.super.startContainers(); | ||
// SharedContainerConfiguration.super.startContainers(); |
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.
Was there a reason this was switched?
modules/testcontainers/build.gradle
Outdated
testImplementation 'org.eclipse.microprofile.rest.client:microprofile-rest-client-api:1.4.1' | ||
testImplementation 'org.slf4j:slf4j-log4j12:1.7.36' | ||
testImplementation 'org.testcontainers:mockserver:1.19.1' | ||
testImplementation project(':microshed-testing-core') |
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.
This should be removed otherwise it will be included as a transitive dependency, but the user needs to choose if they want core
or core-jakarta
testImplementation project(':microshed-testing-core') |
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.
Using testCompileOnly should make it compile and not pass it on as a transitive dep right?
f1caf49
to
5bdc0b0
Compare
Signed-off-by: asjervanasten <[email protected]>
Signed-off-by: asjervanasten <[email protected]>
Signed-off-by: asjervanasten <[email protected]>
Signed-off-by: asjervanasten <[email protected]>
a7011bf
to
d170790
Compare
@KyleAure any changes / suggestions to tackle this? |
I've been working locally to solve a few issues with making such a jump in Gradle versions. I think I'm close will work some more on this tomorrow. |
@KyleAure here's my take on upgrading the Gradle wrapper. However, I'm more experiences in maven, so I've my doubts about this change. At this moment it will not pass CI with a clear error, I'm just not confident on what is the correct approach for now.
Will you review my changes and hopefully solve the problem ocurring in CI?
#311
@KyleAure NVM I solved the issue, CI is now green. Awaiting your review.