-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Upgrade to Spring Cloud Finchley #72 #80
Upgrade to Spring Cloud Finchley #72 #80
Conversation
… 2.0.2, Sleuth 2.0.0.RC2, Zipkin Server 2.8.4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arey thanks for PR, please answer my comments :)
@@ -27,19 +28,20 @@ | |||
* @author Maciej Szarlinski | |||
*/ | |||
@Data | |||
@NoArgsConstructor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need no-args constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember why I have to add the @NoArgsConstructor annotation. I will try to remove it.
@@ -74,7 +75,11 @@ public Owner findOwner(@PathVariable("ownerId") int ownerId) { | |||
@PutMapping(value = "/{ownerId}") | |||
@Monitored | |||
public Owner updateOwner(@PathVariable("ownerId") int ownerId, @Valid @RequestBody Owner ownerRequest) { | |||
final Owner ownerModel = ownerRepository.findOne(ownerId); | |||
final Optional<Owner> owner = ownerRepository.findById(ownerId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
orElseThrow
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's better. Thanks
|
||
|
||
private Pet findPetById(int petId) { | ||
Optional<Pet> pet = petRepository.findById(petId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
orElseThrow
?
Changes applied into the 50231b1 commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixes :)
@mszarlinski may I merge the Fishley branch into master? I've already done a release (and a tag) for the current master version. |
👍 |
…c#80) * spring-petclinic#72 Upgrade to Spring Cloud Finchley M9 * spring-petclinic#72 Upgrade to Spring Boot 2.0.1 * spring-petclinic#72 Remove @deprecated API usage * spring-petclinic#72 Upgrade to Spring Cloud Finchley RC2, Spring Boot 2.0.2, Sleuth 2.0.0.RC2, Zipkin Server 2.8.4 * spring-petclinic#72 Registering the index ViewController is no more required * For logging, implement the toString() method of the Pet entity * spring-petclinic#72 Upgrade to Spring Boot Admin 2.0.0 * spring-petclinic#72 Add @NoArgsConstructor required for Jackson deserialization * spring-petclinic#72 Upgrade to Spring Boot 2.0.3 * spring-petclinic#72 Upgrade to Spring Cloud Finchley RELEASE * Change Zipkin local host URL * spring-petclinic#72 changes requested by the @mszarlinski code review
This pull request upgrades the application to Spring Boot Finchley based on Spring Boot 2.
Local deployment is working.
Docker deployment is working.
By waiting a patch of Spring Boot 2, I have to add the below line into the application.yml of the spring-petclinic-microservices-config module:
For my point of view, the feature/Finchley branch is complete and could be merge into the master. branch. Are you ok?