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

Add option for empty default values #477

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Jan 8, 2025

In Quarkus at static init time we build what is
essentially a no-op CP instance, but creating
this instance currently incurs some overhead
due to the use of Config to look up default values.
This change, will allow Quarkus to set empty
default values option and slightly improve startup

geoand added a commit to geoand/quarkus that referenced this pull request Jan 8, 2025
This is done in order to avoid the overhead of
looking up properties that won't be used anyway

Requires: smallrye/smallrye-context-propagation#477
geoand added a commit to geoand/quarkus that referenced this pull request Jan 8, 2025
This is done in order to avoid the overhead of
looking up properties that won't be used anyway

Requires: smallrye/smallrye-context-propagation#477
Copy link
Contributor

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@FroMage
Copy link
Contributor

FroMage commented Jan 8, 2025

@geoand apparently, we have a requirement for commits to be signed now (first time I hear about this, BTW):

Merging is blocked The base branch requires all commits to be signed. [Learn more about signing commits.](https://docs.github.com/authentication/managing-commit-signature-verification/about-commit-signature-verification)

@geoand
Copy link
Contributor Author

geoand commented Jan 8, 2025

Interesting, let me check and see how that works

geoand added a commit to geoand/quarkus that referenced this pull request Jan 8, 2025
This is done in order to avoid the overhead of
looking up properties that won't be used anyway

Requires: smallrye/smallrye-context-propagation#477
Signed-off-by: Georgios Andrianakis <[email protected]>
@geoand
Copy link
Contributor Author

geoand commented Jan 8, 2025

Should be okay hopefully :)

@FroMage
Copy link
Contributor

FroMage commented Jan 8, 2025

Apparently not :-/

@geoand
Copy link
Contributor Author

geoand commented Jan 8, 2025

I'll try again

@FroMage
Copy link
Contributor

FroMage commented Jan 8, 2025

Your commit does seem to be signed, so I'm not sure that's it.

In Quarkus at static init time we build what is
essentially a no-op CP instance, but creating
this instance currently incurs some overhead
due to the use of Config to look up default values.
This change, will allow Quarkus to set empty
default values option
@geoand geoand force-pushed the empty-default-values branch from 24d2deb to 568440c Compare January 8, 2025 10:47
@geoand
Copy link
Contributor Author

geoand commented Jan 8, 2025

Tried again

@FroMage FroMage merged commit cadaab6 into smallrye:main Jan 8, 2025
5 checks passed
@FroMage FroMage added this to the 2.2.0 milestone Jan 8, 2025
@FroMage FroMage added the enhancement New feature or request label Jan 8, 2025
geoand added a commit to geoand/quarkus that referenced this pull request Jan 8, 2025
This is done in order to avoid the overhead of
looking up properties that won't be used anyway

Requires: smallrye/smallrye-context-propagation#477
Signed-off-by: Georgios Andrianakis <[email protected]>
gsmet pushed a commit to geoand/quarkus that referenced this pull request Jan 9, 2025
This is done in order to avoid the overhead of
looking up properties that won't be used anyway

Requires: smallrye/smallrye-context-propagation#477
Signed-off-by: Georgios Andrianakis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants