Skip to content
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

implemented a workaround for Conul service discovary support. #288

Merged
merged 3 commits into from
Mar 27, 2024

Conversation

raga70
Copy link
Contributor

@raga70 raga70 commented Mar 11, 2024

implemented a workaround for Conul service discovery. Although the solution in this PR works (please take it as just guidance on how to fix it) , I did have time to get fully familiar with your code base and just aplied a hot fix.

PS: my ide made a bit of a reformatting on the files I touched (sorry for the extra hassle when reviewing ), if you have any questions send me a message

… solution in this PR works (please take it as just guidance on how to fix it) , i didnt had time to get fully familiar with your code base and just aplied a hot fix
Copy link

update-docs bot commented Mar 11, 2024

Thanks for opening this pull request! If you have implemented new functions, write about them in the readme file.

@Burgyn
Copy link
Owner

Burgyn commented Mar 14, 2024

Hi @raga70,

thanks for your PR. I can merge this PR if you:

  • fix codefactor bot issues
  • my comment
  • and revert formatting of touched methods

thanks.

@@ -46,6 +46,12 @@ public class DownstreamSwaggerDocsRepository : IDownstreamSwaggerDocsRepository
var clientName = _options.Value.HttpClientName ?? ((route?.DangerousAcceptAnyServerCertificateValidator ?? false) ? ServiceCollectionExtensions.IgnoreSslCertificate : string.Empty);
var httpClient = _httpClientFactory.CreateClient(clientName);

if (!(url.StartsWith("http://") || url.StartsWith("https://"))) //the url that gets constructed when using consul misses the http schema , and trows an exception
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use StringComparison.InvariantCultureIgnoreCase. and remove comments

@raga70
Copy link
Contributor Author

raga70 commented Mar 15, 2024

Hi @Burgyn I committed the changes you requested .

@Burgyn
Copy link
Owner

Burgyn commented Mar 18, 2024

Hi @raga70,

unfortunately your changes failed tests.

Please fix tests.

@raga70
Copy link
Contributor Author

raga70 commented Mar 26, 2024

I added nullability to my ServiceProviderType since the ServiceProviderConfiguration is in fact null when unit testing

@Burgyn Burgyn merged commit c7f6602 into Burgyn:master Mar 27, 2024
3 checks passed
@Burgyn
Copy link
Owner

Burgyn commented Mar 27, 2024

Thanks. This will be release soon as v8.2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants