Conversation
harveymmaunders
force-pushed
the
dotnet-rest-template
branch
2 times, most recently
from
cb5e7ac to
bb79a0a
Compare
harveymmaunders
force-pushed
the
dotnet-rest-template
branch
4 times, most recently
from
509a2c9 to
bda9660
Compare
harveymmaunders
force-pushed
the
dotnet-rest-template
branch
from
d1f5fcd to
f3883c7
Compare
willosborne
left a comment
willosborne
left a comment
There was a problem hiding this comment.
LGTM, will defer to @mgasca for final approval before merge but this seems easy to use and comments/docs are good.
|
A lot of the naming conventions seem very web-like to me e.g. in the service names or the project names. |
client-samples/dotnet/rest/api-template/services/AuthTokenService.cs
Outdated
Show resolved
Hide resolved
|
@ZKRobi this is useful feedback thank you. Will look into these now |
client-samples/dotnet/rest/api-template.tests/services/TestConfigService.cs
Show resolved
Hide resolved
client-samples/dotnet/rest/api-template/services/ConfigService.cs
Outdated
Show resolved
Hide resolved
client-samples/dotnet/rest/api-template/services/ConfigService.cs
Outdated
Show resolved
Hide resolved
mgasca
left a comment
mgasca
left a comment
There was a problem hiding this comment.
Happy to have the 3 nitpick open comments under single review addressed later or not at all, approving PR
Co-authored-by: Miguel <mgasca@gmail.com>
| httpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", token); | ||
|
|
||
| // Make a GET request | ||
| return httpClient.GetAsync(url).Result; |
There was a problem hiding this comment.
I'd propose following through with keeping calls async.
So rather than calling .Result, the method could be "public static async Task GetResponseAsync" and then await the call.
CallApi would then be "public static Task CallApi" and await GetResponse.
| using (X509Certificate2 certificate = GetCertificate(_clientCredentialSettings.PublicKeyFile, _clientCredentialSettings.PrivateKeyFile)) | ||
| { | ||
| var app = CreateConfidentialClient(_clientCredentialSettings.ClientId, GetAuthority(_clientCredentialSettings.Tenant), certificate); | ||
| token = GetToken(app).Result; |
There was a problem hiding this comment.
I'd propose to be consistent with using async everywhere - same as in the ApiService, GetAuthToken could return Task and await here.
| var authTokenService = new AuthTokenService(settings); | ||
|
|
||
| // Retrieve the authentication token synchronously | ||
| var token = authTokenService.GetAuthToken(); |
There was a problem hiding this comment.
Referring to the ApiService and ConfigService comments, if those are made async, here, they would be awaited, and the whole thing would work the same. An implicit main method is async by default, no need to change anything else.
4 participants
No description provided.