Fixed bug with DefaultServiceInstance: getScheme() was not implemented#926
Open
dagerber wants to merge 2 commits intospring-cloud:mainfrom
Open
Fixed bug with DefaultServiceInstance: getScheme() was not implemented#926dagerber wants to merge 2 commits intospring-cloud:mainfrom
dagerber wants to merge 2 commits intospring-cloud:mainfrom
Conversation
This was referenced Mar 12, 2021
OlgaMaciaszek
requested changes
Mar 16, 2021
Collaborator
OlgaMaciaszek
left a comment
OlgaMaciaszek
left a comment
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).
|
|
||
| @Override | ||
| public String getScheme() { | ||
| return this.isSecure() ? "https" : "http"; |
Collaborator
There was a problem hiding this comment.
Please remove unnecessary this. keyword.
| } | ||
|
|
||
| @Override | ||
| public String getScheme() { |
Collaborator
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"); |
Collaborator
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. | |||
Collaborator
|
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)
Collaborator
|
@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. |
Contributor
Author
|
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>
.
|
Collaborator
|
Ok, @dagerber. Please change it then and submit against |
Contributor
Author
|
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>
.
|
Member
|
@dagerber you needed to do a force push as now there are unrelated commits here. |
Contributor
Author
|
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>
.
|
Member
|
Indeed, thanks, that's better. |
Member
|
cherry picked to 2.2.x and merged forward via 4269bb3 |
Member
|
reverted. see #823 (comment) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes the problem, that a https URI could not be overriden in Unit tests with http (avoiding certificate problems on the build server)