-
Notifications
You must be signed in to change notification settings - Fork 213
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
Quarkus LTS Upgrade to 3.8.4 #3477
Changes from all commits
15b6a95
37f9177
dbb472a
b9f7937
e508f18
6a5edf9
62dbba9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,7 +41,7 @@ public class QuarkusTopicDiscovery extends AbstractTopicDiscovery { | |
@Override | ||
protected List<Topic> getTopics() { | ||
final List<Topic> topics = new ArrayList<>(); | ||
ConfigProvider.getConfig().getPropertyNames().forEach(n -> { | ||
getPropertyNames().forEach(n -> { | ||
if (n.startsWith(OUTGOING_PREFIX)) { | ||
final String topicName = this.extractChannelName(n, OUTGOING_PREFIX); | ||
if (topics.stream().noneMatch(t -> t.getName().equals(topicName) && t.getType() == ChannelType.OUTGOING)) { | ||
|
@@ -68,7 +68,15 @@ private String extractChannelName(String property, String prefix) { | |
if (channelName.contains(".")) { | ||
channelName = channelName.substring(0, channelName.indexOf(".")); | ||
} | ||
final Optional<String> topicName = ConfigProvider.getConfig().getOptionalValue(prefix + channelName + TOPIC_SUFFIX, String.class); | ||
final Optional<String> topicName = getOptionalValue(prefix + channelName + TOPIC_SUFFIX); | ||
return topicName.orElse(channelName); | ||
} | ||
|
||
Iterable<String> getPropertyNames() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this method and getOptionalValue should be protected rather than package There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why protected and not default? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I forgot to mention why ;). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For example, when I see the change initially, I believe you have missed the private (later I see you are inheriting in the test), if you put protected, it is clear the purpose of change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as this is in the personal preference realm, I'll leave as is as my personal preference is default scope as it's more restrictive and supposed to be used only by unit test to avoid using environment config |
||
return ConfigProvider.getConfig().getPropertyNames(); | ||
} | ||
|
||
Optional<String> getOptionalValue(String key) { | ||
return ConfigProvider.getConfig().getOptionalValue(key, String.class); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm, are we completely sure that kogito-test-utils is only added as test scoped? if not, we might add with junit being added to set of the dependencies accidentally (probably it is all right, since we would have already realized that assert and rest assure were accidentally being added and it does not seem to be the case)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, why JUnit has to be added to the dependency set? Or the other way around, how this util test was working without this dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't remember what I had to add this, I'll try remove and see how it goes.