Integration of OASP4J Starters #612
Conversation
sjimenez77
requested review from
hohwille,
maybeec,
ivanderk,
sjimenez77 and
EastWindShak
|
This pull request is currently open for review. However the changes in the scope of this pull request are yet to go through a thorough testing phase next week. In case anybody reviews it kindly do not merge until the testing is completed successfully . |
There was a problem hiding this comment.
@AbhayChandel thanks for your work. 👍 This change is highly appreciated. However, I added some review-feedback and would like to request some improvements.
Also I would like to question:
- Do we need a starter for everything? What is the benefit of the logging starter? To be honest it IMHO has actually no added value.
- Please check the dependencies of all starters consistently. I added some review-comments here and there but not in all places. AFAIK you added test dependencies to productive starters. Unless there is some spring-maven-magic I am not aware of this is simply wrong and has to be fixed. Also you have added
spring-boot-autoconfigureto almost every starter POM what I am also unsure about.
Please let us know if you can do the re-work or if someone else has to take over from here. Thanks in advance.
| <version>dev-SNAPSHOT</version> | ||
| </parent> | ||
| <groupId>io.oasp.java.starters</groupId> | ||
| <artifactId>oasp4j-batch-starter</artifactId> |
There was a problem hiding this comment.
Please follow spring conventions and put "starter" keyword before the topic like "batch". See also existing starter modules (and yammer discussions about it):
https://github.com/oasp/oasp4j/tree/develop/modules
| <artifactId>oasp4j-starters</artifactId> | ||
| <version>dev-SNAPSHOT</version> | ||
| </parent> | ||
| <groupId>io.oasp.java.starters</groupId> |
There was a problem hiding this comment.
great that you added a separate structure/folder/parent POM for the starters. But do we want to have a separate groupId for that? Maybe this is causing more problems as users then have to pick the corrent groupId while otherwise the groupId would always be the same for all modules what is IMHO more simple and less error-prune.
starters/pom.xml
Outdated
| <artifactId>oasp4j</artifactId> | ||
| <version>dev-SNAPSHOT</version> | ||
| </parent> | ||
| <artifactId>oasp4j-starters</artifactId> |
There was a problem hiding this comment.
This is great. We should then align this with #611
starters/oasp4j-web-starter/pom.xml
Outdated
| </dependency> | ||
| <dependency> | ||
| <groupId>${oasp.modules.groupId}</groupId> | ||
| <artifactId>oasp4j-test</artifactId> |
There was a problem hiding this comment.
has to be <scope>test</scope> - please fix in all places. IMHO we can drop this dependency from all starters (as long as there are no JUnits in these starters).
starters/oasp4j-web-starter/pom.xml
Outdated
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-autoconfigure</artifactId> |
There was a problem hiding this comment.
Can we add a comment, why we actually need this dependency exactly here?
starters/oasp4j-core-starter/pom.xml
Outdated
| <dependency> | ||
| <groupId>${oasp.modules.groupId}</groupId> | ||
| <artifactId>oasp4j-basic</artifactId> | ||
| <version>${oasp4j.version}</version> |
There was a problem hiding this comment.
Please remove version's from all dependencies in starters and use BOM instead.
|
|
||
| /** | ||
| * @author ABCHANDE | ||
| * |
There was a problem hiding this comment.
See review comment for WebAutoConfiguration
| <dependency> | ||
| <groupId>${oasp.modules.groupId}</groupId> | ||
| <artifactId>oasp4j-batch</artifactId> | ||
| <version>${oasp4j.version}</version> |
There was a problem hiding this comment.
Please remove version's from all dependencies of starters and use BOM instead.
| * | ||
| */ | ||
| @Configuration | ||
| public class JPAAutoConfiguration { |
There was a problem hiding this comment.
Please follow OASP coding conventions:
https://github.com/oasp/oasp4j/wiki/coding-conventions#naming
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework.batch</groupId> | ||
| <artifactId>spring-batch-test</artifactId> |
There was a problem hiding this comment.
This is IMHO wrong. Test dependencies have to be added to the test starter and need to be added to the project with <scope>test</scope>.
|
For security we have planned a new backlog item to introduce actual starters for different authentication setups. So we should have something like I can add a separate issue for that... |
|
|
No description provided.