Handle Spring Boot Profiles #955

kameshsampath · 2017-05-30T07:51:37Z

Fix for #880 (replaces #881)

rhuss

Looks ok except one thing which i believe is a bug. I.e. when you specify multiples profiles in the yaml, I dont think it works.

rhuss · 2017-05-30T08:01:37Z

core/src/main/java/io/fabric8/maven/core/util/SpringBootUtil.java

+                propertiesResource = getResourceFromClasspath(compileClassLoader,
+                        "application-" + profile + ".properties");
+                props2.putAll(getPropertiesResource(propertiesResource));
+                props.putAll(props2);


You could put all of this logic into a method (from line 66 to line 71) and then call it for the case above the if, too (which has the same logic but without a profile name).

I.e. I would add a addApplicationProperties(props, compileClassLoader, profile) with profile == null means no profile.

ok see what you mean

rhuss · 2017-05-30T08:05:17Z

core/src/main/java/io/fabric8/maven/core/util/SpringBootUtil.java

+                        if (profilesValue instanceof Map) {
+                            Map profileMap = (Map) profilesValue;
+                            String[] arrayOfActiveProfiles =
+                                    StringUtils.split((String) profileMap.get("active"), ",");


I would use the same split as below with a regexp here, so you can skip the trim iteration (i.e. activeProfiles.split("\\s*,\\s*") after checking that the map contains an active profile.

Actually why not reuse method getActiveProfiles() here ?

yeah true makes sense, modified the code

rhuss · 2017-05-30T08:13:43Z

core/src/main/java/io/fabric8/maven/core/util/SpringBootUtil.java

+                                intersection(Arrays.asList(profileSplit), activeProfiles)
+                                .isEmpty()) {
+                            //if the profiles is in the list of active profiles we add it to our list of docs
+                            profileDocs.put(profiles, docRoot);


Don't you have to add an entry for every profile ? (not only a single entry for the comma separated list, which is not even normalized)

no thats taken care by previous block, this in case where the yaml has only one doc so ideally we will have only one profile.. i hope my understanding is right

rhuss · 2017-05-30T08:14:58Z

core/src/test/resources/util/test-application-multi.yml

+---
+
+spring:
+  profiles: dev


would be good to have a test with multiple, comma separated profiles, too (as I suspect this is currently broken).

not sure how this will come ??? you mean something like:

spring:
profiles: dev, qa

???

@rhuss - i dont think spring.profiles can have multiple values https://docs.spring.io/spring-boot/docs/current/reference/html/boot-features-profiles.html , so we dont need to worry on it . Only spring.profiles.active can have multiple values via comma seperation

yes, something like this as shown in https://github.com/spring-projects/spring-boot/blob/master/spring-boot-samples/spring-boot-sample-profile/src/main/resources/application.yml

Also, if it could not have multiple values, why did you split it then in the code above ? Also I believe it could be even a YAML list (same like for activeProfiles)

rhuss · 2017-05-30T08:16:20Z

...tor/spring-boot/src/main/java/io/fabric8/maven/generator/springboot/SpringBootGenerator.java

@@ -63,9 +52,18 @@
    private static final String DEFAULT_SERVER_PORT = "8080";

    public enum Config implements Configs.Key {
-        color {{ d = "false"; }};
+        color {{


could we keep the formatting here, as it looks a bit more concise then.

sorry it was my bad when i did formatting IDEA split it as we.. wondering is there way to configure this style ;)

rhuss · 2017-05-30T08:16:30Z

...tor/spring-boot/src/main/java/io/fabric8/maven/generator/springboot/SpringBootGenerator.java


-        public String def() { return d; } protected String d;
+        public String def() {


same here wrt formatting.

rhuss · 2017-05-30T09:21:51Z

core/src/main/java/io/fabric8/maven/core/util/SpringBootUtil.java

+                    }
+
+                    if (profiles != null) {
+                        String[] profileSplit = profiles.split("\\s*,\\s*");


If profiles can take multiple values, why do you split it here then ? And couldn't it be that profiles could be also a list ?

Afaiu activeProfiles is a cascading mechanism where the profiles later in the list override the former.

What's about spring.profiles.include ?

kameshsampath · 2017-06-11T06:56:39Z

@ro14nd - made some logical changes in SpringBootUtil and added test case for include profile as well. I have kept the old logic still in tact, just wanted your thoughts before I clean them up. if you are ok with all logic + additional comments if any, will fix it and clean up unused methods then.

Incorporated review as part of the latest commits

rhuss · 2017-06-11T07:29:05Z

Thanks ! I hope I can do a review today, if not, the next slot is probably only Friday next week ;-(

cmoulliard · 2017-07-13T05:53:39Z

@rhuss Should we rebase this PR to make it work ?

nicolaferraro · 2017-07-17T15:12:43Z

@kameshsampath I've not followed all the evolution of this issue but it seems you've added spring-boot dependency in the last solution. We've made many changes to this plugin in order to get rid of the dependency on spring-boot from the core (it brings also some spring stuff as transitive dependencies).
Ideally we should replicate the same logic used by spring-boot, without importing spring-boot (unless they separate the logic into a tiny module).

Is it possible to restore the old logic, and fix the problems occurring with profiles? (probably with multiple profiles)

…files

* added spring boot dependency to parse the spring boot properties * using spring boot run to make it compute env and retrieved props from it

removed quoutes from -Dspring.profiles.active

This reverts commit f510110.

rhuss · 2018-07-30T20:31:16Z

I rebased this PR and reverted commit e27270b which introduced the spring-boot dependency.

@kameshsampath is this PR still good to merge (even when not used with a spring-boot dep for parsing the files ?). If so, I will try to make a review this week.

And yes, sorry for the looooooong delay ;-)

kameshsampath · 2018-07-31T01:47:53Z

@rhuss am sorry its been really long time, need to check it why I added the spring dep, am not sure I can do this over next few weeks, but will try to take a peep whenever I get time. sorry about it.

rhuss · 2018-07-31T06:41:16Z

@kameshsampath no problem. It was my fault anyway to not finish the review. Let me make another review, and maybe I can polish the PR myself. Lets see ...

kameshsampath · 2018-07-31T06:46:32Z

none's fault @rhuss .. just our timelines/bandwidth just did not synch

kameshsampath mentioned this pull request May 30, 2017

Add profile support in spring-boot #881

Closed

rhuss previously requested changes May 30, 2017

View reviewed changes

rhuss reviewed May 30, 2017

View reviewed changes

kameshsampath force-pushed the spring-boot/issue-880 branch from 7a619b9 to f510110 Compare June 11, 2017 07:01

rhuss self-assigned this Jul 24, 2018

rohanKanojia added the target/3.5 PR targeted to 3.5 label Jul 24, 2018

rhuss changed the title ~~Spring boot/issue 880~~ Allow default values for Spring Boot properties Jul 30, 2018

rhuss changed the title ~~Allow default values for Spring Boot properties~~ Handle Spring Boot Profiles Jul 30, 2018

rhuss force-pushed the spring-boot/issue-880 branch from 817b376 to 2e10a9e Compare July 30, 2018 20:04

kameshsampath and others added 4 commits July 30, 2018 22:23

Added extra config to spring boot generator to handle spring boot pro…

c587d49

…files

Refactor:

e27270b

* added spring boot dependency to parse the spring boot properties * using spring boot run to make it compute env and retrieved props from it

Added dummy spring boot application

5d1ba17

removed quoutes from -Dspring.profiles.active

Revert Commit which introducer spring-boot dependency

942d031

This reverts commit f510110.

rhuss force-pushed the spring-boot/issue-880 branch from 2e10a9e to 942d031 Compare July 30, 2018 20:29

rhuss added WIP target/4.0 PR for targeted to 4.0.x and removed target/3.5 PR targeted to 3.5 labels Jul 31, 2018

rhuss mentioned this pull request Sep 4, 2018

Have the possibility to specify the spring profiles being used #1364

Open

rhuss added pr/wip Work in Progress, do not merge and removed WIP labels Sep 14, 2018

rhuss assigned lordofthejars and unassigned rhuss Oct 1, 2018

devang-gaur added the jkube/pending The issue/PR has to be taken care of in JKube https://github.com/eclipse/jkube label Jul 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle Spring Boot Profiles #955

Handle Spring Boot Profiles #955

kameshsampath commented May 30, 2017 •

edited by rhuss

Loading

rhuss left a comment

rhuss May 30, 2017

kameshsampath May 30, 2017

rhuss May 30, 2017

rhuss May 30, 2017

kameshsampath May 30, 2017

rhuss May 30, 2017

kameshsampath May 30, 2017

rhuss May 30, 2017

kameshsampath May 30, 2017

kameshsampath May 30, 2017

rhuss May 30, 2017

rhuss May 30, 2017

kameshsampath May 30, 2017

rhuss May 30, 2017

kameshsampath May 30, 2017

rhuss May 30, 2017

rhuss May 30, 2017

rhuss May 30, 2017

kameshsampath commented Jun 11, 2017

rhuss commented Jun 11, 2017

cmoulliard commented Jul 13, 2017

nicolaferraro commented Jul 17, 2017

rhuss commented Jul 30, 2018

kameshsampath commented Jul 31, 2018

rhuss commented Jul 31, 2018

kameshsampath commented Jul 31, 2018


		public String def() { return d; } protected String d;
		public String def() {

Handle Spring Boot Profiles #955

Are you sure you want to change the base?

Handle Spring Boot Profiles #955

Conversation

kameshsampath commented May 30, 2017 • edited by rhuss Loading

rhuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kameshsampath commented Jun 11, 2017

rhuss commented Jun 11, 2017

cmoulliard commented Jul 13, 2017

nicolaferraro commented Jul 17, 2017

rhuss commented Jul 30, 2018

kameshsampath commented Jul 31, 2018

rhuss commented Jul 31, 2018

kameshsampath commented Jul 31, 2018

kameshsampath commented May 30, 2017 •

edited by rhuss

Loading