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

Vertx Logger and ConfigProperties was deprecated #215

Merged
merged 1 commit into from
Jun 2, 2021

Conversation

pjgg
Copy link
Contributor

@pjgg pjgg commented May 24, 2021

Upgrade deprecated code:

io.quarkus.arc.config.ConfigProperties was deprecated
Vertx Logger was deprecated

@pjgg pjgg requested a review from jsmrcka May 24, 2021 11:52
@pjgg pjgg force-pushed the feat/remove-deprecated-code branch 4 times, most recently from 5d44df6 to 16bb2bb Compare May 24, 2021 15:53
@pjgg
Copy link
Contributor Author

pjgg commented May 25, 2021

Looks like io.smallrye.config.ConfigMapping doesn't behaves exactly the same as the deprecated io.quarkus.arc.config.ConfigProperties.

ConfigProperties is able to read child properties from a parent class configuration:
Example:

@ConfigProperties(prefix = "antagonist")
public class AntagonistConfiguration {
    public String message;
    public String name;
    public AntagonistWifeConfig wife;

    public static class AntagonistWifeConfig {
        public String name;
        public String message;
    }
}

application.properties

antagonist.name=Sheldon Plankton
antagonist.message=Hi, I am Sheldon Plankton
antagonist.wife.name=Karen
antagonist.wife.message=Hi, I am Karen

But the same scenario with ConfigMapping throw an exception

SRCFG00013: No Converter registered for class io.quarkus.qe.config.AntagonistConfiguration$AntagonistWifeConfig

@rsvoboda
Copy link
Member

@pjgg this is candidate for a bug report. The transition should be smooth for the users.

@pjgg
Copy link
Contributor Author

pjgg commented May 31, 2021

@pjgg this is candidate for a bug report. The transition should be smooth for the users.

Issue: quarkusio/quarkus#17545
Issue: quarkusio/quarkus#17588

Copy link
Contributor

@jsmrcka jsmrcka left a comment

Choose a reason for hiding this comment

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

What is the plan with the different behavior of @ConfigMapping? (See quarkusio/quarkus#17545 (comment), which corresponds with https://smallrye.io/docs/smallrye-config/main/mapping/mapping.html, also referenced in https://quarkus.io/version/main/guides/config#inject-the-configuration.)

I see no way to smoothly transition from the deprecated annotation.

@pjgg pjgg force-pushed the feat/remove-deprecated-code branch from 16bb2bb to 4b5123e Compare June 1, 2021 08:03
@pjgg
Copy link
Contributor Author

pjgg commented Jun 1, 2021

What is the plan with the different behavior of @ConfigMapping? (See quarkusio/quarkus#17545 (comment), which corresponds with https://smallrye.io/docs/smallrye-config/main/mapping/mapping.html, also referenced in https://quarkus.io/version/main/guides/config#inject-the-configuration.)

I see no way to smoothly transition from the deprecated annotation.

The new implementation is great!. To me makes even more sense than the old one from a conceptual point of view.

For example the above code will look something like this:

@ConfigMapping(prefix = "antagonist")
public interface AntagonistConfiguration {
     String message();
     String name();
     AntagonistWifeConfig wife();

    public interface AntagonistWifeConfig {
        String name();
        String message();
    }
}

But the point is that this change is a breaking change that will makes fail all the existing applications that use this feature. So, we should think about a smooth transition.

@pjgg pjgg force-pushed the feat/remove-deprecated-code branch 3 times, most recently from 7153345 to 3e091e8 Compare June 1, 2021 11:10
@pjgg pjgg requested a review from jsmrcka June 1, 2021 16:56
@pjgg pjgg force-pushed the feat/remove-deprecated-code branch from 3e091e8 to 9b99d95 Compare June 2, 2021 08:00
@jsmrcka jsmrcka merged commit 7bfa0f6 into quarkus-qe:main Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants