search

quarkusio / quarkusio.github.io

Website for Quarkus project
https://quarkus.io
156 stars 365 forks source link

Blog post: overriding the config of a Quarkus app from its test code #2026

Closed gwenneg closed 3 days ago

github-actions[bot] commented 1 week ago

🙈 The PR is closed and the preview is expired.

holly-cummins commented 1 week ago

This looks like really useful content!

(I haven't done a proper review because it's something I'm never entirely sure how to do, so I can't provide the right level of technical comments ... but I'm happy this has been written. :) )

gsmet commented 1 week ago

@radcortez would you have the time to have a quick look at this blog post?

radcortez commented 1 week ago

@radcortez would you have the time to have a quick look at this blog post?

I'll have a look.

radcortez commented 1 week ago

Most of the blog is about using @ConfigProperty. I think we shouldn't promote @ConfigProperty as the primary way to inject/retrieve configuration. @ConfigProperty has many limitations, and users should use @ConfigMapping instead.

gwenneg commented 1 week ago

Thanks everyone for reviewing my PR!

@radcortez Could you please share more details about why @ConfigMapping is a better choice than @ConfigProperty?

The purpose of this post is only to talk about ways to override the config values and not how to better inject the configuration. These two topics look a bit orthogonal to me, even though I understand your point. Would it be enough if I added a NOTE at the beginning promoting @ConfigMapping as a better way than @ConfigProperty to inject the config?

gwenneg commented 1 week ago

Shouldn't the Quarkus guides about config be updated to better promote @ConfigMapping?

gwenneg commented 1 week ago

I only updated the post date in the commit I just pushed.

holly-cummins commented 1 week ago

I think one thing this discussion about mapping and property shows is how potentially valuable the kind of content @gwenneg has created will be; we have a bit of an information gap, which means it's maybe easier than it should be for even skilled users to end up using sub-optimal patterns.

gwenneg commented 1 week ago

I may be missing something about @ConfigMapping but I'm not convinced (yet) @ConfigProperty is always a sub-optimal choice.

In the cloud apps I'm responsible for, we:

@ConfigProperty makes all that stuff reasonably easy. I'm not sure how @ConfigMapping would make any of that easier/better.

radcortez commented 1 week ago

@radcortez Could you please share more details about why @ConfigMapping is a better choice than @ConfigProperty?

Sure:

The purpose of this post is only to talk about ways to override the config values and not how to better inject the configuration. These two topics look a bit orthogonal to me, even though I understand your point. Would it be enough if I added a NOTE at the beginning promoting @ConfigMapping as a better way than @ConfigProperty to inject the config?

I understand, and please don't take me wrong. I think it is a great article. @ConfigProperty comes from the MP Config specification, which stopped all the work in favor of Jakarta Config. I feel that we shouldn't be endorsing an API where the current status is unknown at best.

radcortez commented 1 week ago

@ConfigProperty makes all that stuff reasonably easy. I'm not sure how @ConfigMapping would make any of that easier/better.

That is great :) I had no intention to start a debate on how things should be done. If users are happy, I'm also happy :)

gwenneg commented 1 week ago

Thanks for the details @radcortez, that is very interesting. I no longer doubt that @ConfigMapping is better than @ConfigProperty. I'm glad we had that discussion 😄

Is jakarta.config.ConfigMapping supported by Quarkus already?

gwenneg commented 1 week ago

FYI I'm updating the blog post and repo. I'll submit a better version (more @ConfigMapping-oriented) of the post tomorrow.

radcortez commented 1 week ago

Is jakarta.config.ConfigMapping supported by Quarkus already?

We don't have a Jakarta Config release yet. There is a lot of work. Maybe by the end of the year. A few things that we learned about configuration are going into Jakarta Config and @ConfigMapping from SmallRye Config is an example.

BTW, if you end up trying @ConfigMapping more, feel free to reach out and let me know if you find any gaps. I'm happy to help :)

gwenneg commented 1 week ago

@radcortez I just pushed a big post update which gives more visibility to config mappings. @ConfigProperty is still here but always comes second. Please let me know if that looks better.

radcortez commented 1 week ago

@radcortez I just pushed a big post update which gives more visibility to config mappings. @ConfigProperty is still here but always comes second. Please let me know if that looks better.

Thank you for the change. Really appreciate it.

gwenneg commented 5 days ago

@radcortez I pushed another change related to profile aware files. Could you please confirm that what I wrote is correct? I'm not 100% sure about the details related to src/main/resources vs src/test/resources in native mode. I tested everything in this repo and then transformed the result of my tests into the addition to the blog post.

radcortez commented 3 days ago

@radcortez I pushed another change related to profile aware files. Could you please confirm that what I wrote is correct? I'm not 100% sure about the details related to src/main/resources vs src/test/resources in native mode. I tested everything in this repo and then transformed the result of my tests into the addition to the blog post.

Yes, the native image build only considers main sources. You don't want your test sources to be included in the image that goes to prod :)

gwenneg commented 3 days ago

@gsmet Do you want to take another look before this is merged? A lot of things changed in the post since your first review.

gwenneg commented 3 days ago

Thanks everyone for the reviews and @radcortez in particular for helping me make that post better!