fix: do not output warning when resolving a configuration #2892
Conversation
Signed-off-by: Chris Laprun <[email protected]>
| void test() { | ||
| final var service = new BaseConfigurationService(); | ||
| final var config = service.getConfigurationFor(new NoDepReconciler()); | ||
| System.out.println(config); |
There was a problem hiding this comment.
This test should be asserting the logged message or removed.
There was a problem hiding this comment.
Ah yes, this wasn't supposed to end up in the commit… 😅
I thought about asserting the lack of logging but this would require changing the architecture (i.e. make the logger an instance variable instead of a class one) just for the tests so decided against it.
There was a problem hiding this comment.
if is does not test anything, should we remove the test?
| * prevent automated, on-demand creation of a reconciler's configuration, please use the | ||
| * {@link AbstractConfigurationService} implementation instead as base for your extension. | ||
| */ | ||
| @Deprecated(forRemoval = true) |
There was a problem hiding this comment.
Somewhat strange to introduce a new usage of the method and deprecate it at the same time. How will this be handled when this method is removed?
There was a problem hiding this comment.
The current implementation uses the method but the documentation explains what should happen instead. It's only deprecated to avoid API breakage in case implementations use it but this method should really be removed, because, as far as I'm aware, there is no implementation that doesn't currently return true and the creation scenario should only be confined to this particular implementation (or its subclasses) anyway.
| "Configuration for reconciler '{}' was not found. {}", | ||
| reconcilerKey, | ||
| reconcilersNameMessage); | ||
| if (!createIfNeeded()) { |
There was a problem hiding this comment.
what is the probelm we are solving this method evetually?
There was a problem hiding this comment.
I mean in quarkus operator sdk
There was a problem hiding this comment.
Right now, the issue is preserving the possibility to log (or do something else, if needed) if the configuration doesn't exist in the abstract class, without logging when the configuration is created in BaseConfigurationService. Removing the logging and createIfNeeded methods would be API breaking changes so the goal is to have an implementation that doesn't break the existing API and doesn't log a warning when a configuration is created on-demand.
Signed-off-by: Chris Laprun <[email protected]>
Signed-off-by: Chris Laprun [email protected]