Add profile support in spring-boot #881
Conversation
Codecov Report
@@ Coverage Diff @@
## master #881 +/- ##
===========================================
- Coverage 22.37% 18.6% -3.78%
+ Complexity 580 491 -89
===========================================
Files 164 160 -4
Lines 8684 8403 -281
Branches 1530 1502 -28
===========================================
- Hits 1943 1563 -380
- Misses 6529 6648 +119
+ Partials 212 192 -20 |
rhuss
left a comment
rhuss
left a comment
There was a problem hiding this comment.
@kamesh sorry, sorry for the long delay (shame on me :(. No excuse, but were busy time (and I couldn't work much on any other f-m-p issue, too).
In principle the PR, however I have some comments (see below) and I wonder what we should do with other advanced features like includes.
I really would like to have a check how spring-boot itself does the parsing, maybe we can avoid to reinvent the wheel.
| Properties props = getPropertiesFromYamlResource(ymlResource,activeProfiles); | ||
| props.putAll(getPropertiesResource(propertiesResource)); | ||
|
|
||
| if(activeProfiles!=null){ |
There was a problem hiding this comment.
minor thing, but it would be awesome if you could stick to the spacing conventions of the projects (e.g. spaces between braces and operators)
| ymlResource = compileClassLoader.findResource("application-"+profile+".yml"); | ||
| Properties props2 = getPropertiesFromYamlResource(ymlResource,activeProfiles); | ||
| propertiesResource = compileClassLoader.findResource("application-"+profile+".properties"); | ||
| props2.putAll(getPropertiesResource(propertiesResource)); |
There was a problem hiding this comment.
The code looks duplicated to the snippet above. Please extract this is into an extra method.
There was a problem hiding this comment.
i have moved to new method - not sure that what you really meant.
| Properties properties = new Properties(); | ||
| if (source != null) { | ||
| properties.putAll(getFlattenedMap(source)); | ||
|
|
There was a problem hiding this comment.
The following code block is quite large. Could we split it up in some smaller chunks by moving parts into methods ?
It's really hard to follow and understand what it does with all the different state variables. would be very awesome if this would be easier to understand (tbh I'm not sure whether I fully grasped it)
I even wouldn't mind if we would have two passes through the document, one to detect the active profiles and the other to parse the profiles. I think this would it much easier to understand and we would need to cover less corner cases.
|
|
||
| if (profilesValue instanceof LinkedHashMap) { | ||
| Map porfMap = (Map) profilesValue; | ||
| String[] actProfiles = StringUtils.split((String) porfMap.get("active"), ","); |
There was a problem hiding this comment.
what's about whitespaces before and after commas ? So maybe we should .trim() the profile names before putting them into the profile list.
| activeProfiles.addAll(Arrays.asList(actProfiles)); | ||
| } | ||
| } else if (profilesValue instanceof String) { | ||
| profiles = (String) profilesValue; |
There was a problem hiding this comment.
Why don't you process the logic by splitting up the profiles string right here ?
| //Spring boot active profiles | ||
| String strActiveProfiles = getConfig(activeProfiles); | ||
| if(strActiveProfiles!=null) { | ||
| opts.add("-Dspring.profiles.active="+strActiveProfiles); |
There was a problem hiding this comment.
Better embedd it in quotes or escape spaces when "activeProfiles" contain spaces (like in "a, b")
There was a problem hiding this comment.
quoted it - but we need to see how it works hen deployed in k8s, sometimes i see it does not work as expected ..
| String strActiveProfiles = getConfig(activeProfiles); | ||
|
|
||
| Properties properties = SpringBootUtil.getApplicationProperties(getContext().getProject(), | ||
| SpringBootUtil.getActiveProfiles(strActiveProfiles)); |
There was a problem hiding this comment.
Same as above, please use a singe method in SpringBootUtils.
There was a problem hiding this comment.
new method added
| String strActiveProfiles = getConfig(activeProfiles); | ||
|
|
||
| Properties properties = SpringBootUtil.getApplicationProperties(getContext().getProject(), | ||
| SpringBootUtil.getActiveProfiles(strActiveProfiles)); |
There was a problem hiding this comment.
the same, one method please.
There was a problem hiding this comment.
new method added
| project.getPlugin(anyString); result = null; | ||
| project.getVersion(); result = "1.0.0"; minTimes = 0; | ||
| context.getConfig(); result = config; | ||
| config.getConfig("spring-boot","activeProfiles");result="dev,qa"; |
There was a problem hiding this comment.
would be cool to have a test with spaces after the comma, too.
There was a problem hiding this comment.
added new test and mock context with spaces in profiles
| Properties properties = SpringBootUtil.getSpringBootApplicationProperties(getContext().getProject()); | ||
| String strActiveProfiles = getContext().getConfig().getConfig("spring-boot","activeProfiles"); | ||
|
|
||
| Properties properties = SpringBootUtil.getApplicationProperties(getContext().getProject(), |
There was a problem hiding this comment.
same again, one method instead of two.
There was a problem hiding this comment.
new method added
|
since i need the old branch for logic reference shall we use the the PR #955 |
2 participants
Support for adding Spring Boot profiles as JAVA_OPTIONS when the user provides configuration via
activeProfilesability to handle multiple application yaml docs with/without spring active profiles
This pull resolves #797 and avoids #880 which can now be customised via profile during build