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

Ensure that InitialConfigurator uses the Quarkus configured min log level #27826

Merged
merged 1 commit into from Sep 10, 2022

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Sep 9, 2022

This essentially prevents issues like #27735 where a piece of Quarkus code executing very early in the startup sequence, would improperly determine the minimum logging level - i.e. ALL was used instead of the Quarkus build configured minimum level.

@quarkus-bot quarkus-bot bot added area/core area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins labels Sep 9, 2022
@mkouba
Copy link
Contributor

mkouba commented Sep 9, 2022

I wonder if it would make sense to write a test for this? I.e. it's possible to test a static_init recorder (although it's not very nice) so in theory it could be feasible... OTOH I'm not sure it's worth the effort ;-)

@geoand geoand force-pushed the 27735-followup branch 2 times, most recently from 36c045a to 9a261f1 Compare September 9, 2022 08:34
@geoand
Copy link
Contributor Author

geoand commented Sep 9, 2022

I wonder if it would make sense to write a test for this? I.e. it's possible to test a static_init recorder (although it's not very nice) so in theory it could be feasible... OTOH I'm not sure it's worth the effort ;-)

I personally don't think it's worth the effort, although if someone wants to add a test, I won't object :)

@mkouba
Copy link
Contributor

mkouba commented Sep 9, 2022

I wonder if it would make sense to write a test for this? I.e. it's possible to test a static_init recorder (although it's not very nice) so in theory it could be feasible... OTOH I'm not sure it's worth the effort ;-)

I personally don't think it's worth the effort, although if someone wants to add a test, I won't object :)

I don't insist, it was just an idea ;-)

@Sanne
Copy link
Member

Sanne commented Sep 9, 2022

Wondering if bootstrap times improve a little :)
Probably negligible but this mistake seems so easy to be made that it's bound to have had some additional effects we don't know of.

@Sanne Sanne added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Sep 9, 2022
@geoand
Copy link
Contributor Author

geoand commented Sep 9, 2022

Wondering if bootstrap times improve a little :)

I wouldn't hold my breath for this :)

this mistake seems so easy to be made that it's bound to have had some additional effects we don't know of.

Absolutely true

@quarkus-bot

This comment has been minimized.

@geoand
Copy link
Contributor Author

geoand commented Sep 9, 2022

The native test failure seems related. I'll have to have a look on Monday

…evel

This essentially prevents issues like quarkusio#27735
where a piece of Quarkus code executing very early in the startup sequence,
would improperly determine the minimum logging level - i.e. ALL was used
instead of the Quarkus build configured minimum level.
@geoand
Copy link
Contributor Author

geoand commented Sep 9, 2022

PR fixed

@geoand geoand merged commit 68cad6a into quarkusio:main Sep 10, 2022
@quarkus-bot quarkus-bot bot added this to the 2.13 - main milestone Sep 10, 2022
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Sep 10, 2022
@geoand geoand deleted the 27735-followup branch September 12, 2022 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants