typesafehub / conductr-lib

Other
8 stars 13 forks source link

Initial Lagom 1.4 support #162

Closed longshorej closed 6 years ago

longshorej commented 7 years ago
longshorej commented 7 years ago

Lagom 1.4 for conductr-lib

longshorej commented 7 years ago

Thanks for the reviews! I addressed the feedback and pushed another commit. I'm going to test once more with Chirper and then ask for a final review.

longshorej commented 7 years ago

Manual test was completed successfully.

TimMoore commented 6 years ago

There might be a regression in the Scala version of Lagom 1.4 support in conductr-lib. There was previously some refactoring of the way that circuit breakers are wired in, and it seems like this was not preserved in the new artifacts. As a result, adding with ConductRApplicationComponents to an application loader (as currently documented) no longer compiles unless you also add with CircuitBreakerComponents.

These changes went back and forth so many times, however, that I'm getting a bit lost trying to follow the progress. I can't tell whether this is an intentional change or not.

If we do want to preserve source compatibility by making this work as currently documented, then I think we'll need ConductRServiceLocatorComponents to extend CircuitBreakerComponents in the 1.4 module. Does that sound right? If everyone agrees, I can send a pull request.

Previous references:

ATTN: @ignasi35, @renatocaval, @longshorej, @fsat

fsat commented 6 years ago

@TimMoore - I agree - I think it sounds like a reasonable thing to do.

fsat commented 6 years ago

PR #167 raised to address https://github.com/typesafehub/conductr-lib/pull/162#issuecomment-351240570.