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.

Sign up for GitHub

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

Jump to bottom

Disable DevService if docker is not running #16959

Merged
merged 1 commit into from
May 4, 2021
Merged

Disable DevService if docker is not running #16959

merged 1 commit into from
May 4, 2021

Conversation

glefloch
Copy link
Member

@glefloch glefloch commented May 3, 2021

This branch adds a check that looks for a running docker daemon. If it's not present, the DevServicesDatasourceProcessor build step is not triggered.

close #16904

@quarkus-bot quarkus-bot bot added the area/core label May 3, 2021
@glefloch glefloch requested review from gastaldi and geoand May 3, 2021 20:51
stuartwdouglas
stuartwdouglas requested changes May 3, 2021
View reviewed changes
Copy link
Member

@stuartwdouglas stuartwdouglas left a comment

Choose a reason for hiding this comment

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

Can we also get a better error message that mentions that docker is an option: https://github.com/stuartwdouglas/quarkus/blob/e7fc52bf892d4507a42cacf1e418feb81c570b39/extensions/agroal/runtime/src/main/java/io/quarkus/agroal/runtime/DataSources.java#L126

Basically if you are in dev or test mode with a DS that supports it it should say 'configure the URL or get a working docker', while for prod mode or for DB's that don't support it the message should stay the same.

gastaldi
gastaldi reviewed May 3, 2021
View reviewed changes
core/deployment/src/main/java/io/quarkus/deployment/util/DockerWorking.java Outdated
@@ -11,8 +11,6 @@

import org.jboss.logging.Logger;

import io.quarkus.deployment.util.ExecUtil;

public class DockerWorking implements BooleanSupplier {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you rename this class to IsDockerRunning to match the existing BooleanSupplier classes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also move it to the io.quarkus.deployment package

@glefloch
Copy link
Member Author

glefloch commented May 4, 2021

Thanks for your reviews, I updated the code to address them :)

machi1990
machi1990 requested changes May 4, 2021
View reviewed changes
Copy link
Member

@machi1990 machi1990 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Let's also disable it for the mongo extension dev services

quarkus/extensions/mongodb-client/deployment/src/main/java/io/quarkus/mongodb/deployment/DevServicesMongoProcessor.java

Lines 39 to 42 in ba39f5d

@BuildStep(onlyIfNot = IsNormal.class)
public DevServicesMongoResultBuildItem startMongo(List<MongoConnectionNameBuildItem> mongoConnections,
MongoClientBuildTimeConfig mongoClientBuildTimeConfig,
LaunchModeBuildItem launchMode,

@quarkus-bot
Copy link

quarkus-bot bot commented May 4, 2021 •
edited
Loading

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building cf8f8ca

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
⌛ Initial JDK 11 Build Persist Maven Repo ⚠️ Check → Logs Raw logs
🚫 Devtools Tests - JDK ${{ matrix.java.name }}
🚫 Devtools Tests - JDK 11 Windows
🚫 Gradle Tests - JDK 11 ${{ matrix.os.family }}
🚫 JVM Tests - JDK ${{ matrix.java.name }}
🚫 JVM Tests - JDK 11 Windows
🚫 Maven Tests - JDK ${{ matrix.java.name }}
🚫 Maven Tests - JDK 11 Windows
🚫 MicroProfile TCKs Tests
🚫 Native Tests - ${{ matrix.category }}
🚫 Native Tests - Windows - ${{ matrix.category }}

@glefloch
Copy link
Member Author

glefloch commented May 4, 2021

@machi1990 done!

geoand
geoand approved these changes May 4, 2021
View reviewed changes
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Looks nice, thanks!

@quarkus-bot
Copy link

quarkus-bot bot commented May 4, 2021 •
edited
Loading

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building 1524af2

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
⌛ Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs
🚫 Devtools Tests - JDK ${{ matrix.java.name }}
🚫 Devtools Tests - JDK 11 Windows
🚫 Gradle Tests - JDK 11 ${{ matrix.os.family }}
🚫 JVM Tests - JDK ${{ matrix.java.name }}
🚫 JVM Tests - JDK 11 Windows
🚫 Maven Tests - JDK ${{ matrix.java.name }}
🚫 Maven Tests - JDK 11 Windows
🚫 MicroProfile TCKs Tests
🚫 Native Tests - ${{ matrix.category }}
🚫 Native Tests - Windows - ${{ matrix.category }}

machi1990
machi1990 approved these changes May 4, 2021
View reviewed changes
Copy link
Member

@machi1990 machi1990 left a comment

Choose a reason for hiding this comment

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

LGTM

gastaldi
gastaldi approved these changes May 4, 2021
View reviewed changes
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

Great job!

@quarkus-bot
Copy link

quarkus-bot bot commented May 4, 2021

Failing Jobs - Building 2fd78ff

Status Name Step Test failures Logs Raw logs
✔️ JVM Tests - JDK 11
✖ JVM Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 15

@glefloch
Copy link
Member Author

glefloch commented May 4, 2021

I'm not sure the CI failure is related..

@gastaldi
Copy link
Contributor

gastaldi commented May 4, 2021

I'm not sure the CI failure is related..

Doesn't seem to be

@gastaldi gastaldi merged commit de90ad4 into quarkusio:main May 4, 2021
@quarkus-bot quarkus-bot bot added this to the 2.0 - main milestone May 4, 2021
@glefloch glefloch deleted the fix/16904 branch May 4, 2021 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@machi1990 machi1990 machi1990 approved these changes

@geoand geoand geoand approved these changes

@stuartwdouglas stuartwdouglas stuartwdouglas approved these changes

@gastaldi gastaldi gastaldi approved these changes

Assignees
No one assigned
Labels
area/core area/mongodb
Projects
None yet
Milestone
2.0.0.Alpha2
Development

Successfully merging this pull request may close these issues.

Devservices must disabled themself if docker is not installed
5 participants
@glefloch @gastaldi @stuartwdouglas @geoand @machi1990