Skip to content

spring-boot:run The usage of -Dspring.profiles.active=... should be discouraged #16819

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

Closed
drahkrub opened this issue May 10, 2019 · 7 comments
Closed
Labels
status: duplicate A duplicate of another issue

Comments

@drahkrub
Copy link

drahkrub commented May 10, 2019

Proposal for an improvement

As you can read in my off-topic follow-up questions in #16811 (comment) I ran into some problems due to the fact that the JVM was forked because of using the parameter workingDirectory.

Some more thoughts: (cut/paste from the aforementioned issue)

Ok, I think, I got it: There are several pitfalls when defining active profiles in Spring-Boot:

  1. When working with the Spring-Boot Maven plugin the usage of -Dspring.profiles.active=... should IMHO produce a big fat warning on screen, because it simply does not work if the JVM is forked which can implicitely happen when using specific parameters.
  2. -Dspring-boot.run.profiles=... instead is the right way to go, but it can only be used with the Spring-Boot Maven plugin. It implicitely creates the command line argument --spring.profiles.active=... which in return must be consumed by SpringApplication#run(...), therefore using run(...) without args should produce a big fat warning in the logs, too.
  3. When starting a JAR- or WAR-file then using --spring.profiles.active=... is problematic because of the aforementioned reasons. In this case the best way to go is using -Dspring.profiles.active=..., I think.

Looks a litte bit messy... ;-)

Would be interested to hear your opinion about that!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 10, 2019
@philwebb
Copy link
Member

When working with the Spring-Boot Maven plugin the usage of -Dspring.profiles.active=... should IMHO produce a big fat warning on screen, because it simply does not work if the JVM is forked which can implicitly happen when using specific parameters.

There certainly seem to be a few comments about this on #1095 so I wonder if we can check if the run process is going to be forked and then print warnings if the current JVM has spring.profile system properties defined.

Therefore using run(...) without args should produce a big fat warning in the logs, too.

This one is a little harder because the run method takes a varargs array. There's not going to be any difference between run(MyApp.class) and run(MyApp.class, args) if incoming args are empty. The can't really think of a good way to do what you're proposing.

@philwebb philwebb added the for: team-attention An issue we'd like other members of the team to review label May 14, 2019
@drahkrub
Copy link
Author

There certainly seem to be a few comments about this on #1095 so I wonder if we can check if the run process is going to be forked and then print warnings if the current JVM has spring.profile system properties defined.

(spring.profiles.active) That would be an improvement! But I think if a VM is forked, it would be good to always point that out explicitly (in the logs or better on screen).

Therefore using run(...) without args should produce a big fat warning in the logs, too.

This one is a little harder because the run method takes a varargs array. There's not going to be any difference between run(MyApp.class) and run(MyApp.class, args) if incoming args are empty. The can't really think of a good way to do what you're proposing.

Hm, I see. Maybe using varargs in this places have not been a good idea. ;-) SpringApplication#run(...) is always(?) called from a main method so that String[] args is always available - so why making it optional by using varargs in SpringApplication#run(...)?

But ok - if one want's to change that at all - that's hard to change. One could only deprecate all SpringApplication#run(...) methods and introduce a new method name e.g. start using String[] args instead of String ...args - but I'm quite sure, that will not happen... ;-)

@philwebb
Copy link
Member

SpringApplication#run(...) is always(?) called from a main method so that String[] args is always available

I'm afraid that's not the case. The API is commonly used that way, but there's no hard requirement that it's always triggered from a main methods. That's one reason why var-args were used.

@drahkrub
Copy link
Author

I'm afraid that's not the case. The API is commonly used that way, but there's no hard requirement that it's always triggered from a main methods. That's one reason why var-args were used.

Ok, I see, than there's nothing to do, I guess - which leaves item 1 on my proposals...

@snicoll
Copy link
Member

snicoll commented May 15, 2019

should IMHO produce a big fat warning on screen, because it simply does not work if the JVM is forked which can implicitely happen when using specific parameters.

FTR this was considered before and is not an oversight.

That line of argument can be used for any property of any plugin forking a process. If we start producing a warning for a specific property, why not also do it for another? (you're warning us about X but not about Y). You may also have that property around and be bothered by getting a warning every time you start your app. We can use an info message and a generic message with the downside that is producing noise if you are aware of that behaviour and you might miss the message if you don't.

@drahkrub
Copy link
Author

That line of argument can be used for any property of any plugin forking a process.

Correct, that's why I wrote "always point that (VM was forked) out explicitly".

We can use an info message and a generic message with the downside that is producing noise if you are aware of that behaviour and you might miss the message if you don't.

True, but I think the advantages outweigh the disadvantages.

To sum it up: I'm aware of the aforementioned "pitfalls" now, but it cost me quite amount of time searching. A warning or info on screen could prevent that for future users.

@snicoll
Copy link
Member

snicoll commented May 21, 2019

True, but I think the advantages outweigh the disadvantages.

I am not sure about that but regardless this request is now a duplicate of #10926

@snicoll snicoll closed this as completed May 21, 2019
@snicoll snicoll added status: duplicate A duplicate of another issue and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate A duplicate of another issue
Projects
None yet
Development

No branches or pull requests

4 participants