search

jhipster / generator-jhipster

JHipster is a development platform to quickly generate, develop, & deploy modern web applications & microservice architectures.
https://www.jhipster.tech
Apache License 2.0
21.27k stars 4k forks source link

Use Maven/Gradle profile(s) as default Spring profile(s) #3224

Closed erikkemperman closed 8 years ago

erikkemperman commented 8 years ago

Sorry for the many PRs, I will stop now :-)

As discussed, among other places, in issue #3180, this promotes the profiles used with Maven/Gradle to be the default profile(s) in Spring -- unless an explicit spring.profiles.active is given, of course.

This means that mvn -Pprod package && java -jar target/foo.war does what you'd expect without the need for arguments. Likewise when deploying the war.

It might be too late for the 3.0 release...? Although I kind of feel that it belongs together with the change that made the dev and prod assets mutually exclusive in the war file.

I did not change the default profile to be different for run or package goals, as was suggested: I am not sure this is even possible with Maven.

I briefly looked into changing the war filename depending on the profiles used, as was suggested in the linked issue, but the generators for AWS, Cloudfoundry, and Heroku seem to only expect a single war file to be present: Cloudfoundry and Heroku use *.war, while AWS (arbitrarily?) picks the last *.war.original file returned by fs.readdirSync.

By the way, shouldn't the uploaded war for those generators always be the .original one, also for Heroku and Cloudfoundry?

deepu105 commented 8 years ago

I know for sure that .original us required for AWS but not sure of heroku and CF. I can check CF but someone needs to check heroku On 20 Mar 2016 05:56, "Erik Kemperman" notifications@github.com wrote:

Sorry for the many PRs, I will stop now :-)

As discussed, among other places, in issue #3180 https://github.com/jhipster/generator-jhipster/issues/3180, this promotes the profiles used with Maven/Gradle to be the default profile(s) in Spring -- unless an explicit spring.profiles.active is given, of course.

This means that mvn -Pprod && java -jar target/foo.war does what you'd expect without the need for arguments. Likewise when deploying the war.

It might be too late for the 3.0 release...? Although I kind of feel that it belongs together with the change that made the dev and prod assets mutually exclusive in the war file.

I did not change the default profile to be different for run or package goals, as was suggested: I am not sure this is even possible with Maven.

I briefly looked into changing the war filename depending on the profiles used, as was suggested in the linked issue, but the generators for AWS, Cloudfoundry, and Heroku seem to only expect a single war file to be present: Cloudfoundry and Heroku use .war, while AWS (arbitrarily?) picks the last .war.original file returned by fs.readdirSync.

By the way, shouldn't the uploaded war for those generators not always be

the .original one, also for Heroku and Cloudfoundry?

You can view, comment on, or merge this pull request online at:

https://github.com/jhipster/generator-jhipster/pull/3224 Commit Summary

  • Use Maven/Gradle profile(s) as default Spring profile(s)

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/jhipster/generator-jhipster/pull/3224

deepu105 commented 8 years ago

And yes this should be part if 3.0 On 20 Mar 2016 11:14, d4udts@gmail.com wrote:

I know for sure that .original us required for AWS but not sure of heroku and CF. I can check CF but someone needs to check heroku On 20 Mar 2016 05:56, "Erik Kemperman" notifications@github.com wrote:

Sorry for the many PRs, I will stop now :-)

As discussed, among other places, in issue #3180 https://github.com/jhipster/generator-jhipster/issues/3180, this promotes the profiles used with Maven/Gradle to be the default profile(s) in Spring -- unless an explicit spring.profiles.active is given, of course.

This means that mvn -Pprod && java -jar target/foo.war does what you'd expect without the need for arguments. Likewise when deploying the war.

It might be too late for the 3.0 release...? Although I kind of feel that it belongs together with the change that made the dev and prod assets mutually exclusive in the war file.

I did not change the default profile to be different for run or package goals, as was suggested: I am not sure this is even possible with Maven.

I briefly looked into changing the war filename depending on the profiles used, as was suggested in the linked issue, but the generators for AWS, Cloudfoundry, and Heroku seem to only expect a single war file to be present: Cloudfoundry and Heroku use .war, while AWS (arbitrarily?) picks the last .war.original file returned by fs.readdirSync.

By the way, shouldn't the uploaded war for those generators not always be

the .original one, also for Heroku and Cloudfoundry?

You can view, comment on, or merge this pull request online at:

https://github.com/jhipster/generator-jhipster/pull/3224 Commit Summary

  • Use Maven/Gradle profile(s) as default Spring profile(s)

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/jhipster/generator-jhipster/pull/3224

deepu105 commented 8 years ago

I had a look and im not sure. I dont like having additional xml and a class for this, isnt there a simpler way?

Im not sure if this additional complexity which is a bit non standard for normal spring apps IMO is worth the value provided which is letting the user run without passing a profile.

Any way we need to see if there is any simpler alternative to this

erikkemperman commented 8 years ago

Absolutely agree that this is not very pretty! This is why I made it a hidden file -- slightly embarrassing :-)

I have tried to find a nicer way to do this, but couldn't find one. If anyone has ideas... Please let me know!

For what it's worth though, putting this in the application.yml definitely won't work; it has to be done before spring parses those yaml files, because it will be used to determine which of those to load.

erikkemperman commented 8 years ago

Of course the same mechanism, with almost no extra effort, could now be used to deliver more properties from build to runtime. For my own application I'm thinking to include build tool (maven/gradle), build date, git revision (hash of latest commit), ...

deepu105 commented 8 years ago

May be lets take some time and see what can be done, this is not a blocker for 3.0 anyway so we can take our time

Thanks & Regards, Deepu

On Sun, Mar 20, 2016 at 3:52 PM, Erik Kemperman notifications@github.com wrote:

Of course the same mechanism, with almost no extra effort, could now be used to deliver more properties from build to runtime. For my own application I'm thinking to include build tool (maven/gradle) date, git revision (hash of latest commit), ...

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/jhipster/generator-jhipster/pull/3224#issuecomment-198866795

deepu105 commented 8 years ago

lets do this after 3.0 so im gonna put this on hold

erikkemperman commented 8 years ago

Well, despite maybe being subject to a better implementation, I think that it functionally accomplishes a useful detail -- and it shouldn't get in the way of anything else, I don't think, contact surface with rest of the code is unobtrusive and limited to the main and webxml classes.

But fair enough, please let me know if / when to pick this up, or if anyone sees better ways to do this!

erikkemperman commented 8 years ago

Maybe I should mention one other implementation I considered, but decided against: the @variable@ subs could of course be done on (a) Java file(s) directly, without an extra xml file or properties class. But we were interpolating/filtering the xml files anyway (for @loglevel@) and having the class seems preferable as it's a natural place to handle fallback for unsubstited values (e.g running in IDE).

deepu105 commented 8 years ago

lets wait and see if someone has a better idea

Thanks & Regards, Deepu

On Sun, Mar 20, 2016 at 5:39 PM, Erik Kemperman notifications@github.com wrote:

Maybe I should mention one other implementation I considered, but decided against: the @variable@ subs could of course be done on (a) Java file(s) directly, without an extra xml file or properties class. But we were interpolating/filtering the xml files anyway (for @loglevel@) and having the class seems preferable as it's a natural place to handle fallback for unsubstited values (e.g running in IDE).

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/jhipster/generator-jhipster/pull/3224#issuecomment-198884490

erikkemperman commented 8 years ago

Minor FYI: removed a now unneeded import from ApplicationWeb.xml, and rebased onto latest master (had to adapt to a commit to make gradle/protractor test pass).

I would still love to hear if anyone has ideas on how to improve on this implementation!

erikkemperman commented 8 years ago

Travis is flaky; restarted the failing test and now it passed.

deepu105 commented 8 years ago

@jhipster/developers guys we need to progress on this, any comments from you guys?

atomfrede commented 8 years ago

Looks good to me. I have no feeling against xml, in case it stays as small as possible

PierreBesson commented 8 years ago

I'm in favor of including this. It will be great for on-boarding because users would expect the app to start in prod when packaging a jar for prod. Also if spring.profiles.active always take precedence it should not cause issues. It will also be very helpful as we started to make more use of profiles recently with no-swagger and no-liquibase.

erikkemperman commented 8 years ago

Rebased again and fixed an inaccurate comment in BuildProperties.java.

deepu105 commented 8 years ago

Im not a spring boot expert but I was wondering. Spring boot does support loading stuff from an external application.properties or application.yml file as stated here https://docs.spring.io/spring-boot/docs/current/reference/html/boot-features-external-config.html

may be we could figure something out using that and the springs various annotations to load specific stuff based on things available in classpath or environment. Thats how spring does most of the autoconfiguration magic isnt it?

erikkemperman commented 8 years ago

Well, I'm no spring expert either, so maybe I'm missing something...

But my thinking was we can't use spring's autoconfig voodoo for this particular case, because we need it done before spring looks for external .properties or .yaml files. This is because the profile we are loading here will be used to determine which external config files spring must load, e.g. application-dev.yml or application-prod.yml.

I could change it from .build-properties.xml to a regular properties file if you prefer, I chose xml just because the maven pom was already set up to do filtering (which is its odd name for what is actually substitution) on xml files.

But I think we'd still need to load it manually, and provide a fallback for unsubstuted values, which makes me think having a separate class (BuildProperties.java) is actually a pretty natural fit for the purpose.

deepu105 commented 8 years ago

Lets see what @jdubois has to say

Thanks & Regards, Deepu

On Fri, Apr 1, 2016 at 3:15 PM, Erik Kemperman notifications@github.com wrote:

Well, I'm no spring expert either, so maybe I'm missing something...

But my thinking was we can't use spring's autoconfig voodoo for this particular case, because we need it done before spring looks for external .properties or .yaml files. This is because the profile we are loading here will be used to determine which external config files spring must load, e.g. application-dev.yml or application-prod.yml.

I could change it from .build-properties.xml to a regular properties file if you prefer, I chose xml just because the maven pom was already set up to do filtering (which is its odd name for what is actually substitution) on xml files.

But I think we'd still need to load it manually, and provide a fallback for unsubstuted values, which makes me think having a separate class ( BuildProperties.java) is actually a pretty natural fit for the purpose.

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/jhipster/generator-jhipster/pull/3224#issuecomment-204280684

jdubois commented 8 years ago

Yes Spring first reads all those profiles, so you can't modify them afterwards (or you need to refresh your app context, but that's another story). I'm not a big fan of having the same name for the WAR file both in dev and prod, as then it's very easy to pick up the wrong version. I'd like to have a "-dev.war" and "-prod.war" file generated, depending on the Maven profile used to do the packaging. Yes we do use ".war" in some sub-generators, as the War file might as been changed by the user (it depends on the app name), but we can still do a "-dev.war" and "*-prod.war" instead.

erikkemperman commented 8 years ago

Maybe the war name ahould be separate from this PR?

Like I said I looked into it but several cloudy generators would need work as well, they don't expect more than one war file to exist.

jdubois commented 8 years ago

Yes but we should change those sub generators.

erikkemperman commented 8 years ago

I'd propose dealing with war names separately, for the moment I think the question for this PR was if there is a better way to pass maven/gradle profile to spring?

erikkemperman commented 8 years ago

Rebased and fixed a conflict that arose due to recent commit(s) on master.

I'm curious how to proceed here... It seems that functionally the consensus is that this would be a useful addition, but I haven't seen any suggestions as to how to improve the implementation?

deepu105 commented 8 years ago

@erikkemperman sorry again to let you hang in like this. Gimme some time to test this out

erikkemperman commented 8 years ago

No worries, it's just that I have to keep rebasing because I have a project based off this branch which I want to keep up to date with JH master, at least until the next minor release (which will hopefully have this, or something like this, in it).

Thanks for taking the time, whenever you find some!

deepu105 commented 8 years ago

Ok so i tried few other ways and I guess whatever is said here doesnt completely work in our case and hence Im ok to proceed with this PR half hearted. So @jhipster/developers if no body has an objection we can merge this.

@erikkemperman thanks for hanging on with us :)

erikkemperman commented 8 years ago

Thanks for looking into this!

I'm a little uncomfy with your "half-hearted" remark, wouldn't want to inflict anything on you guys you're not happy with... So can I ask if that's just cosmetic, aesthetic considerations? Or are you actually worried if it might cause functional headaches?

deepu105 commented 8 years ago

For now its just aesthetics :) On 9 Apr 2016 22:48, "Erik Kemperman" notifications@github.com wrote:

Thanks for looking into this!

I'm a little uncomfy with your "half-hearted" remark, wouldn't want to inflict anything on you guys you're not happy with... So can I ask if that's just cosmetic, aesthetic considerations? Or are you actually worried if it might cause functional headaches?

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/jhipster/generator-jhipster/pull/3224#issuecomment-207798819

deepu105 commented 8 years ago

@jdubois we can merge this if you are ok

erikkemperman commented 8 years ago

I don't mean to be too forward... But any chance of getting this in soon? I am working up to a release and would much prefer not to base it off my own branch. Thanks in advance!

deepu105 commented 8 years ago

Im waiting for @jdubois to take a look. He is in vacation rite now

jdubois commented 8 years ago

No I'm back :-) But lots of "real" work, and there's Devoxx Fr next week... I don't have much time for coding on JHipster at the moment.

jdubois commented 8 years ago

I've had a look:

Another option would be to only generate a production build - anyway I don't think there's a valid use case for doing a "dev" WAR file. We used to have this because we couldn't have source maps in JavaScript, but that's solved now.

erikkemperman commented 8 years ago

@jdubois Running in IDE works same as before, it will see unsubstituted @placeholders@ and fall back to old behavior.

It seems everyone thinks the xml file is not pretty (myself included!) but I haven't seen proposals to do it more elegantly. Thoughts?

I agree that the single war name is potentially confusing... Either foo-dev.war and foo-prod.war or just defaulting to prod would work for me.

But I think that is outside the scope of this PR and probably better addressed separately?

deepu105 commented 8 years ago

I guess some teams might want to have a dev war file and also even if we always build a prod war how will we set the prod profile as default for that it will still have the same issue as today and might need the same solution as this PR On 20 Apr 2016 00:29, "Erik Kemperman" notifications@github.com wrote:

@jdubois https://github.com/jdubois Running in IDE works same as before, it will see unsubstituted @placeholders https://github.com/placeholders@ and fall back to old behavior.

It seems everyone thinks the xml file is not pretty (myself included!) but I haven't seen proposals to do it more elegantly. Thoughts?

I agree that the single war name is potentially confusing... Either foo-dev.war and foo-prod.war or just defaulting to prod would work for me.

But I think that is outside the scope of this PR and probably better addressed separately?

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/jhipster/generator-jhipster/pull/3224#issuecomment-212005732

jdubois commented 8 years ago

So Deepu you want to merge it? Who also likes this PR? I don't want to be the bad guy holding on stuff everyone else wants! Le 20 avr. 2016 2:27 AM, "Deepu K Sasidharan" notifications@github.com a écrit :

I guess some teams might want to have a dev war file and also even if we always build a prod war how will we set the prod profile as default for that it will still have the same issue as today and might need the same solution as this PR On 20 Apr 2016 00:29, "Erik Kemperman" notifications@github.com wrote:

@jdubois https://github.com/jdubois Running in IDE works same as before, it will see unsubstituted @placeholders https://github.com/placeholders@ and fall back to old behavior.

It seems everyone thinks the xml file is not pretty (myself included!) but I haven't seen proposals to do it more elegantly. Thoughts?

I agree that the single war name is potentially confusing... Either foo-dev.war and foo-prod.war or just defaulting to prod would work for me.

But I think that is outside the scope of this PR and probably better addressed separately?

— You are receiving this because you commented. Reply to this email directly or view it on GitHub < https://github.com/jhipster/generator-jhipster/pull/3224#issuecomment-212005732

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/jhipster/generator-jhipster/pull/3224#issuecomment-212183199

deepu105 commented 8 years ago

@jdubois I'm hesitant as well but I don't see a better way to do this. either we need to merge this or let the users always set the profile manually

erikkemperman commented 8 years ago

FYI Rebased again to resolve conflicts with commits to master branch.

erikkemperman commented 8 years ago

FYI Rebased again... Any news on this PR?

notflorian commented 8 years ago

This PR looks good to me. :+1: @jdubois @deepu105 : Would you be OK to merge it, or do you think it need more changes?

jdubois commented 8 years ago
erikkemperman commented 8 years ago

@jdubois What kind of issues are you expecting? I've been working with a project with this patch included and for my use cases everything is fine. Note that it has basically zero contact surface with the rest of the code.

The custom XML processing (well, not really, it just uses the standard Properties.loadFromXml method) is necessary. Or at least, we can't use Spring's autoconfig voodoo, for reasons discussed several times in this thread.

The thread seems to be getting somewhat circular -- you keep bringing up the naming of the war files and I agree but would again suggest that this is a different topic and out of scope for this PR :-)

Anyway, either way is fine with me. I'm going to have to make a first release of my project this week. It would have been nice if this PR was in the main jhipster repository, or even better if it were part of a minor release... But I guess that's not going to happen, fair enough.

So, feel free to do with this what you like, but I might not be re-basing it all the time anymore to follow jhipster/master.

jdubois commented 8 years ago

Sorry for repeating myself, it's just that their are more comments on this thread, and my opinion still hasn't changed from the beginning, so of course I'm always answering the same thing to the same comments. I still don't think having our own XML file, and reading it from the classpath, is a good idea. Then, yes I agree our current process isn't good, and it should be changed.

erikkemperman commented 8 years ago

Well I totally agree it's not pretty -- although I don't expect much trouble other than sore eyes -- and I've certainly been open to suggestions for improvement. It seems most folks agree it is ugly but there haven't been any more elegant proposals, unfortunately.

Not sure what you mean by the "process isn't good"?

Also, I just noticed I had missed a key part in your earlier comment:

[with proper war file names...] this change wouldn't be needed

Maybe I misunderstand, but you'd still have to do --spring.active.profiles=... right? Are we sure we have the same image of what this PR does / was supposed to do?

jdubois commented 8 years ago

"process isn't good" -> I agree the way it currently work isn't good, we need a better solution "with proper war file names" -> my idea here is that by default we package the right profile with each war file. So wouldn't have to add --spring.active.profiles with a -prod.war file, as it would already be its default profile. Sorry I'm just lacking time on this part, we have 30 tickets opened and a lot of work, and I'm mostly doing this on my free time.

erikkemperman commented 8 years ago

I would be curious to see how setting the default profile would work -- that is pretty much what I was trying to do here -- so I'll keep an eye on this!

No worries, I certainly didn't mean to sound impatient! Your efforts are much appreciated by many here, I'm sure... It's just that this PR has been in limbo for a while now :-)

jdubois commented 8 years ago

Thanks, I just need some time to review tickets, but there are so many of them at the moment...

deepu105 commented 8 years ago

@jdubois @erikkemperman let me step in to clear few things.

@jdubois even with having the profile in war names I dont think there is a clean way to set the default spring profile and thats the very reason for this PR and like you I dont like the XML based solution and had a long discussion with @erikkemperman.

Hence naming pattern of war doesn't have anything to do here, our problem from the very beginning was setting a default spring profile and we always asked the user to do that manually and either we can set it using this PR or continue to ask the user to do it manually like earlier.

I tried to do it with spring default profile property but doesnt seem to work, but as per spring documents if we set that in the application.yml its suppose to work. But somehow it doesnt.

erikkemperman commented 8 years ago

@deepu105 Thanks, I think that's all accurate. Wanted to point out though: it's not a choice anymore, since the war contains only one profile in terms of front end code, the user has to pass the same profile used while making the war.