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

Use @ConfigMapping in SmallRye extensions #45757

Merged
merged 5 commits into from
Jan 22, 2025

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Jan 21, 2025

Created as draft to do a last review with the diff and check the whole CI.

Probably better reviewed commit per commit.

Part of #45446

Copy link

quarkus-bot bot commented Jan 21, 2025

/cc @ebullient (metrics), @jmartisk (metrics)

public Optional<Boolean> openapiIncluded;
@WithName("openapi.included")
@WithDefault("false")
boolean openapiIncluded();
Copy link
Member Author

Choose a reason for hiding this comment

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

For a weird reason, the config was split in two classes. I centralized everything in one class.

Comment on lines +10 to +11
@ConfigRoot(phase = ConfigPhase.BUILD_TIME)
@ConfigMapping(prefix = "quarkus.rabbitmq")
Copy link
Member Author

Choose a reason for hiding this comment

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

@ozangunalp @cescoffier the config for the Messaging extensions is rather inconsistent. Some of them have quarkus.messaging.whatever and some of them quarkus.whatever. I would have expected the consistent quarkus.messaging prefix.

It's out of the scope of this PR though and I will let you handle it once this is in if you want to adjust.

Copy link

github-actions bot commented Jan 21, 2025

🎊 PR Preview 7bf5b30 has been successfully built and deployed to https://quarkus-pr-main-45757-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

Copy link
Member

@radcortez radcortez left a comment

Choose a reason for hiding this comment

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

I've seen a few places using @ConfigGroup which shouldn't be required anymore.


/**
* Abort a query if the total depth of the query exceeds the defined limit. Default to no limit
*/
@ConfigItem
public Optional<Integer> instrumentationQueryDepth;
Optional<Integer> instrumentationQueryDepth();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Optional<Integer> instrumentationQueryDepth();
OptionalInt instrumentationQueryDepth();

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 addressed all of them.


/**
* Abort a query if the total number of data fields queried exceeds the defined limit. Default to no limit
*/
@ConfigItem
public Optional<Integer> instrumentationQueryComplexity;
Optional<Integer> instrumentationQueryComplexity();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Optional<Integer> instrumentationQueryComplexity();
OptionalInt instrumentationQueryComplexity();


/**
* The maximum number of raw whitespace tokens the parser will accept, after which an exception will be thrown. Default to
* 200000
*/
@ConfigItem
public Optional<Integer> parserMaxWhitespaceTokens;
Optional<Integer> parserMaxWhitespaceTokens();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Optional<Integer> parserMaxWhitespaceTokens();
OptionalInt parserMaxWhitespaceTokens();


/**
* The maximum number of raw tokens the parser will accept, after which an exception will be thrown. Default to 15000
*/
@ConfigItem
public Optional<Integer> parserMaxTokens;
Optional<Integer> parserMaxTokens();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Optional<Integer> parserMaxTokens();
OptionalInt parserMaxTokens();

@gsmet
Copy link
Member Author

gsmet commented Jan 22, 2025

I've seen a few places using @ConfigGroup which shouldn't be required anymore.

@radcortez yeah I know we have a disagreement on it :). I personally prefer being explicit, both for documentation sake (you know it's a config group when you see it and because I'm not a big fan of magic - I have been hit by the bus too many times later).

@gsmet gsmet force-pushed the smallrye-config-mapping branch from dccdf78 to b64aef0 Compare January 22, 2025 08:54
gsmet added 4 commits January 22, 2025 10:03
Also drop the deprecated configuration (it's present in at least one LTS
as it has been deprecated in 3.14) and rationalize the config classes
(we had two config classes with the exact same scope).
Also extract the config to a proper file.
@gsmet gsmet force-pushed the smallrye-config-mapping branch from b64aef0 to b329543 Compare January 22, 2025 09:05
@gsmet gsmet marked this pull request as ready for review January 22, 2025 09:05
@gsmet
Copy link
Member Author

gsmet commented Jan 22, 2025

This is now ready for review.

Copy link

quarkus-bot bot commented Jan 22, 2025

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit b329543.

✅ 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.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

@radcortez
Copy link
Member

@radcortez yeah I know we have a disagreement on it :). I personally prefer being explicit, both for documentation sake (you know it's a config group when you see it and because I'm not a big fan of magic - I have been hit by the bus too many times later).

Ok :)

Copy link

quarkus-bot bot commented Jan 22, 2025

Status for workflow Quarkus CI

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

✅ 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 21

📦 extensions/infinispan-cache/deployment

io.quarkus.cache.infinispan.InfinispanCacheTest.testGetAsyncWithParallelCalls - History

  • expected: "thread1" but was: "thread2" - org.opentest4j.AssertionFailedError
org.opentest4j.AssertionFailedError: 

expected: "thread1"
 but was: "thread2"
	at io.quarkus.cache.infinispan.InfinispanCacheTest.testGetAsyncWithParallelCalls(InfinispanCacheTest.java:283)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:513)
	at io.quarkus.test.QuarkusUnitTest.interceptTestMethod(QuarkusUnitTest.java:427)

📦 integration-tests/observability-lgtm

io.quarkus.observability.test.LgtmLifecycleTest.testTracing - History

  • java.lang.RuntimeException: java.lang.reflect.InvocationTargetException - java.lang.RuntimeException
java.lang.RuntimeException: java.lang.RuntimeException: java.lang.reflect.InvocationTargetException
	at io.quarkus.test.junit.QuarkusTestExtension.throwBootFailureException(QuarkusTestExtension.java:611)
	at io.quarkus.test.junit.QuarkusTestExtension.interceptTestClassConstructor(QuarkusTestExtension.java:695)
	at java.base/java.util.Optional.orElseGet(Optional.java:364)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
Caused by: java.lang.RuntimeException: java.lang.reflect.InvocationTargetException
	at io.quarkus.test.junit.TestResourceUtil$TestResourceManagerReflections.startReflectively(TestResourceUtil.java:214)

@gsmet gsmet merged commit 3f6f477 into quarkusio:main Jan 22, 2025
62 of 64 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.19 - main milestone Jan 22, 2025
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.

3 participants