-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Some cleanup for config server 3 #1617
Some cleanup for config server 3 #1617
Conversation
Configure Renovate
@@ -205,19 +214,7 @@ public static MultipleSourcesContainer processNamedData(List<StrippedSourceConta | |||
rawData = decodeData(rawData); | |||
} | |||
|
|||
// In some cases we want to include properties from the default profile |
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.
tbh, I'm not changing this code that is shown here as removed, what I am doing is refactor it a bit. As a matter of fact, logically, it does not change one bit.
What I want to achieve is this: make it more readable, but more importantly test it, a lot. Besides the possibility to test it a lot, I am leaving myself a door to comment on each individual test and what it is supposed to achieve and why.
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 think keeping the comment though is important still
@@ -51,6 +54,12 @@ public final class ConfigUtils { | |||
|
|||
private static final Log LOG = LogFactory.getLog(ConfigUtils.class); | |||
|
|||
// sourceName (configmap or secret name) ends with : "-dev.yaml" or the like. |
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.
what I do change very little is the introduction of this BiPredicate
that instead that contains
that we talked about on a previous PR, uses endsWith
. imho, this is easier to read and understand
@ryanjbaxter ready one more to look at. Thank you |
@@ -227,13 +224,34 @@ public static MultipleSourcesContainer processNamedData(List<StrippedSourceConta | |||
return new MultipleSourcesContainer(foundSourceNames, data); | |||
} | |||
|
|||
static boolean processSource(boolean includeDefaultProfileData, Environment environment, String sourceName, | |||
Map<String, String> sourceRawData) { | |||
Set<String> activeProfiles = Arrays.stream(environment.getActiveProfiles()).collect(Collectors.toSet()); |
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.
Can we use a List here? The order of profiles is generally important so I want to make sure we maintain that order
@@ -205,19 +214,7 @@ public static MultipleSourcesContainer processNamedData(List<StrippedSourceConta | |||
rawData = decodeData(rawData); | |||
} | |||
|
|||
// In some cases we want to include properties from the default profile |
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 think keeping the comment though is important still
No description provided.