Fixed bug with DefaultServiceInstance: getScheme() was not implemented #926
Conversation
There was a problem hiding this comment.
@dagerber Thanks for submitting this. In general, it looks good, however, have added some comments to address. Also, please add your name with the @author tag in the classes you've changed.
Since this issue is also present in the Hoxton release train, please submit this PR against the 2.2.x branch instead (either edit this PR or create a new one).
| @@ -173,4 +173,9 @@ public int hashCode() { | |||
| return Objects.hash(instanceId, serviceId, host, port, secure, metadata); | |||
| } | |||
|
|
|||
| @Override | |||
| public String getScheme() { | |||
| return this.isSecure() ? "https" : "http"; | |||
There was a problem hiding this comment.
Please remove unnecessary this. keyword.
| @@ -173,4 +173,9 @@ public int hashCode() { | |||
| return Objects.hash(instanceId, serviceId, host, port, secure, metadata); | |||
| } | |||
|
|
|||
| @Override | |||
| public String getScheme() { | |||
There was a problem hiding this comment.
Please reuse this method in getUri(ServiceInstance instance).
|
|
||
| @Test | ||
| public void testGetANonExistentServiceShouldReturnAnEmptyList() { | ||
| then(this.discoveryClient.description()).isEqualTo("Simple Reactive Discovery Client"); |
There was a problem hiding this comment.
Please remove the use of unnecessary this. keywords in tests.
| @@ -0,0 +1,102 @@ | |||
| /* | |||
| * Copyright 2012-2020 the original author or authors. | |||
|
Fixes gh-823. |
This fixes the problem, that a https URI could not be overriden in Unit tests with http (avoiding certificate problems on the build server)
|
@dagerber since we are doing a release today, I'm going to cherry-pick your commits, add the changes from the comments and merge it already. |
|
Hi Olga
I just started fixing, would be done in 30min
Do you want to wait?
Daniel
Am Di., 16. März 2021 um 16:51 Uhr schrieb Olga Maciaszek-Sharma <
***@***.***>:
… @dagerber <https://github.com/dagerber> since we are doing a release
today, I'm going to cherry-pick your commits, add the changes from the
comments and merge it already.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#926 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEEOD2NMSB5UTRNZDEECNLTD547PANCNFSM4ZB4DMDA>
.
|
|
Ok, @dagerber. Please change it then and submit against |
|
Hi Olga
Please cherry-pick as you suggested before.
It takes too much time to fix against 2.2.x
Thanks! Daniel
Am Di., 16. März 2021 um 16:59 Uhr schrieb Olga Maciaszek-Sharma <
***@***.***>:
… Ok, @dagerber <https://github.com/dagerber>. Please change it then and
submit against 2.2.x.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#926 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEEOD7PDYFKN3BY2TXHSOTTD557DANCNFSM4ZB4DMDA>
.
|
|
@dagerber you needed to do a force push as now there are unrelated commits here. |
|
Sorry about that. I reset --hard to the commit before the unwanted merge
and push forced....
Hope it is OK now
Am Di., 16. März 2021 um 17:50 Uhr schrieb Spencer Gibb <
***@***.***>:
… @dagerber <https://github.com/dagerber> you needed to do a force push as
now there are unrelated commits here.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#926 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEEOD64QEZPPQ5T44F44KTTD6D55ANCNFSM4ZB4DMDA>
.
|
|
Indeed, thanks, that's better. |
|
cherry picked to 2.2.x and merged forward via 4269bb3 |
|
reverted. see #823 (comment) |
This fixes the problem, that a https URI could not be overriden in Unit tests with http (avoiding certificate problems on the build server)