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.

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

Move logging to @ConfigMapping #42157

Merged
merged 1 commit into from
Oct 22, 2024
Merged

Move logging to @ConfigMapping #42157

merged 1 commit into from
Oct 22, 2024

Conversation

radcortez
Copy link
Member

@radcortez radcortez commented Jul 26, 2024

It would help to come up with fixes for:

Noticeable changes:

@radcortez radcortez requested a review from famod July 26, 2024 10:04
@radcortez radcortez marked this pull request as ready for review July 26, 2024 12:34
@quarkus-bot quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/gradle Gradle labels Jul 26, 2024
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@radcortez radcortez requested review from gsmet and removed request for famod July 30, 2024 15:21
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Thanks! Let's not merge this too quickly.

I wonder if 3.14 is the right version to merge this, or it would be better to wait for 3.16 to get branched.

For now, I implemented a workaround to support mixed extensions.

@radcortez
Copy link
Member Author

Sure

@radcortez
Copy link
Member Author

On a separate discussion, I wonder if we should introduce another config phase, BUILD_AND_RUN_TIME, which means values are read and used both on build and runtime. Probably only logging requires this, but maybe there are other use cases.

@geoand
Copy link
Contributor

geoand commented Aug 1, 2024

There is also a conflict in this

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Let's try to get this in for 3.16.

I rebased to get a fresh CI run.

@radcortez can you just have a look at my question?

quarkus.log.file.path=quarkus.log
Copy link
Member

Choose a reason for hiding this comment

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

This was a mistake in the config file or something changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe there is a mistake in the configuration file. The quarkus.log.file is a group:

@ConfigDocSection
public FileConfig file;

Also, there is no property with the parent's name.

@radcortez
Copy link
Member Author

Sure. Thank you!

@quarkus-bot

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Oct 22, 2024

I fixed an import conflict. Let's wait for CI and merge!

@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 22, 2024
@radcortez
Copy link
Member Author

Thanks!

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 22, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 9eff425.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/opentelemetry/deployment

io.quarkus.opentelemetry.deployment.metrics.HttpServerMetricsTest.collectsHttpRouteFromEndAttributes - History

  • Assertion condition defined as a Lambda expression in io.quarkus.opentelemetry.deployment.common.exporter.InMemoryMetricExporter expected: <true> but was: <false> within 5 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a Lambda expression in io.quarkus.opentelemetry.deployment.common.exporter.InMemoryMetricExporter expected: <true> but was: <false> within 5 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.untilAsserted(ConditionFactory.java:790)
	at io.quarkus.opentelemetry.deployment.common.exporter.InMemoryMetricExporter.assertCountPointsAtLeast(InMemoryMetricExporter.java:131)
	at io.quarkus.opentelemetry.deployment.common.exporter.InMemoryMetricExporter_ClientProxy.assertCountPointsAtLeast(Unknown Source)

📦 extensions/smallrye-reactive-messaging/deployment

io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector - History

  • Expecting actual: ["-4","-5","-6","-7","-8","-9","-10","-11"] to start with: ["-3", "-4", "-5", "-6"] - java.lang.AssertionError
java.lang.AssertionError: 

Expecting actual:
  ["-4","-5","-6","-7","-8","-9","-10","-11"]
to start with:
  ["-3", "-4", "-5", "-6"]

	at io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector(ConnectorChangeTest.java:36)
  • Expecting actual: ["-4","-5","-6","-7","-8","-9","-10","-11"] to start with: ["-3", "-4", "-5", "-6"] - java.lang.AssertionError
java.lang.AssertionError: 

Expecting actual:
  ["-4","-5","-6","-7","-8","-9","-10","-11"]
to start with:
  ["-3", "-4", "-5", "-6"]

	at io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector(ConnectorChangeTest.java:36)
  • Expecting actual: ["-4","-5","-6","-7","-8","-9","-10","-11"] to start with: ["-3", "-4", "-5", "-6"] - java.lang.AssertionError
java.lang.AssertionError: 

Expecting actual:
  ["-4","-5","-6","-7","-8","-9","-10","-11"]
to start with:
  ["-3", "-4", "-5", "-6"]

	at io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector(ConnectorChangeTest.java:36)

@gsmet gsmet merged commit ae67ce8 into quarkusio:main Oct 22, 2024
52 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 22, 2024
@quarkus-bot quarkus-bot bot added this to the 3.17 - main milestone Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Core module is using a mix of traditional @ConfigRoot and @ConfigMapping
3 participants