search

ErnestOrt / Trampoline

Admin Spring Boot Locally
http://ernestort.github.io/Trampoline/
Apache License 2.0
356 stars 81 forks source link

Use Spring Poot port from application.yml/boostrap.yml #25

Closed BohdanKorinnyi closed 7 years ago

BohdanKorinnyi commented 7 years ago

@ErnestOrt

ErnestOrt commented 7 years ago

Hi @BohdanKorinnyi,

First of all, thanks for your effort!

Doing a first quick review, before performing a test, I've notice the following:

Constants.java

Move this static class to an enum and rename it with a more specific name for instance FileExtension.

pomLocation

This field should not contain neither pom.xml or build.gradle, if we do do check if pom.xml is in the path (which I see it a good idea) we should also check that build.gradle is not there.

ResponseEntity

Should not be more simple to throw an exception if configuration file has not been found instead of using Optional & ResponseEntity?

Missing requirement...

This is an improvement which we did not think and after review it makes a lot of sense. Based on which build file has been found (pom.xml or buil.gradle) we could already set the build tool. Also we should add an initial option on the Register Microservice dropdown form called select build tool to avoid having maven choosed by default, and do the proper validation on the javascript side.

Cheers, Ernest

ErnestOrt commented 7 years ago

Hi @BohdanKorinnyi, functionality doesn't work even with the two microservices that we have as example. I will have to reject this pull request. I encourage you to take your time when developing a feature, it's not a competition and we need to be accurate with simplicity and readability.