-
Notifications
You must be signed in to change notification settings - Fork 76
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
Issue 367 #394
Issue 367 #394
Conversation
Generally speaking, this allows the user to define the prop like:
And the environment variables If the default values are not exist, the prop should be defined like this:
The colon should not be omitted because it simplify the regexp processing pattern and ensure the efficiency. @chengfang Do you agree with this solution? |
@liweinan we can limit the change to jberet-se classes like BatchSEEnvironment.java? Since this new feature will only be available for JBeret in Java SE. jberet-core classes like JdbcRepository should be able to remain intact. mongodb job repository also uses db-user and db-password, so having these changes in BatchSEEnvironment.java will also cover mongo repository. From a user's point of view, it is a bit surprise to be required to use : in the expression. This is not what users do the similar expression in other environment so if possible, I would keep it consistent with everywhere else by not requiring the : |
@chengfang Okay! I'll do the modification accordingly. |
@chengfang I have refactored the impl according to your suggestion, could you please help to review it again? Thanks! |
@chengfang Thanks for the suggestion! I have updated the PR accordingly. Could you please help to to review it again? If it's okay I'll squash the commits. |
@chengfang Thanks for the suggestions! Today is too late I'll finish updating this PR during the weekend :D |
jberet-se/src/test/java/org/jberet/se/BatchSEEnvironmentTest.java
Outdated
Show resolved
Hide resolved
jberet-se/src/test/java/org/jberet/se/BatchSEEnvironmentTest.java
Outdated
Show resolved
Hide resolved
@chengfang I have finished updating this PR according to your review comments, could you please help to review it again? Thanks! |
jberet-se/src/test/java/org/jberet/se/BatchSEEnvironmentTest.java
Outdated
Show resolved
Hide resolved
Thanks for checking @chengfang I'll do the cleanup and merge this. |
…or jberet.properties jberet#367
No description provided.