[ADMINAPI-1297] - Implements Tenant Endpoints on V2 #361
Conversation
Test Results284 tests 283 ✅ 10s ⏱️ Results for commit 11e720a. ♻️ This comment has been updated with latest results. |
DavidJGapCR
force-pushed
the
ADMINAPI-1297
branch
3 times, most recently
from
4739fce to
d8cb78c
Compare
| { | ||
| [SwaggerSchema(Description = AdminConsoleConstants.TenantIdDescription, Nullable = false)] | ||
| public int TenantId { get; set; } | ||
| [SwaggerSchema(Description = AdminConsoleConstants.TenantIdDescription, Nullable = false, Format = "{\r\n \"name\": \"Tenant1\",\r\n \"edfiApiDiscoveryUrl\": \"https://api.ed-fi.org/v7.2/api\"\r\n }")] |
There was a problem hiding this comment.
Although this is just an example, let's use 7.3 (latest) here instead of 7.2, please.
| { | ||
| _log.WarnFormat("Tenant {Key} has an invalid connection string for database {ADMIN_DB_KEY}. Database engine is {engine}", | ||
| tenantConfig.Key, ADMIN_DB_KEY, _appSettings.AppSettings.DatabaseEngine); | ||
| } |
There was a problem hiding this comment.
I'm curious about this... why validating this connection string here?
If validating the Admin database, why not validating the Security database as well?
|
|
||
| public override async Task StopAsync(CancellationToken cancellationToken) | ||
| { | ||
| _log.Info("Stopping background"); |
There was a problem hiding this comment.
What if there are multiple background processes running?
There was a problem hiding this comment.
Talked to Juan about this class to confirm we no longer need it. I will remove it.
|
|
||
| namespace EdFi.Ods.AdminApi.Infrastructure.Services; | ||
|
|
||
| public class TenantBackgroundService : BackgroundService |
There was a problem hiding this comment.
This class would benefit from an XML comment describing its purpose.
| } | ||
| dynamic document = new ExpandoObject(); | ||
| document.edfiApiDiscoveryUrl = tenantConfig.Value.EdFiApiDiscoveryUrl; | ||
| document.name = tenantConfig.Key; |
There was a problem hiding this comment.
I wonder if we could have a regular DTO instead of an ExpandoObject here?
What do you think about including part of the connection string in this payload? There are probably only two parameters to return: the server and database.
- NPGSQL connection string
hostandportdatabase
- MSSQL connection string
serveror alternative formdata sourcedatabaseor alternative forminitial catalog
|
|
DavidJGapCR
force-pushed
the
ADMINAPI-1297
branch
2 times, most recently
from
9c41859 to
ec9b89a
Compare
🔍 Vulnerabilities of
|
| digest | sha256:f527b7f7d15b7e81adb73a9c9466a35efb6545875eaf850c2c965bc8c1fb161d |
| vulnerabilities |     |
| platform | linux/amd64 |
| size | 94 MB |
| packages | 587 |
📦 Base Image alpine:3
| also known as |
|
| digest | sha256:33735bd63cf84d7e388d9f6d297d348c523c044410f553bd878c6d7829612735 |
| vulnerabilities |   |
Description | ||||||||
Description
Description |
DavidJGapCR
force-pushed
the
ADMINAPI-1297
branch
8 times, most recently
from
2859a02 to
1f1c513
Compare
DavidJGapCR
force-pushed
the
ADMINAPI-1297
branch
from
b5ecdee to
7b7102f
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements tenant endpoints on V2 API by creating a new tenant management system that replaces the previous AdminConsole-based implementation. The changes remove the AdminConsole functionality entirely and introduce native V2 tenant endpoints for CRUD operations.
- Removes AdminConsole module and related configurations
- Implements new V2 tenant endpoints (GET, POST, DELETE) with proper validation
- Updates Docker configurations and CI workflows to remove AdminConsole dependencies
- Adds comprehensive unit and integration tests for the new tenant functionality
Reviewed Changes
Copilot reviewed 91 out of 92 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/http/tenants.http | New HTTP file demonstrating V2 tenant API usage |
| Application/EdFi.Ods.AdminApi/Features/Tenants/* | New V2 tenant feature implementation with models, endpoints, and validation |
| Application/EdFi.Ods.AdminApi/Infrastructure/Services/Tenants/* | Core tenant service implementation and file-based configuration management |
| Application/EdFi.Ods.AdminApi/Infrastructure/Database/Commands/* | Commands for adding and deleting tenants from configuration |
| Application/EdFi.Ods.AdminApi.V1/Features/Tenants/* | V1 tenant endpoints for backward compatibility |
| Application/EdFi.Ods.AdminApi.AdminConsole/* | Removed entire AdminConsole module |
| Docker/* | Updated Docker configurations to remove AdminConsole references |
| Application/EdFi.Ods.AdminApi.UnitTests/Features/Tenants/* | Comprehensive unit tests for new tenant functionality |
| Console.WriteLine("Error"); | ||
| Console.WriteLine(ex); |
There was a problem hiding this comment.
Debug code should not be committed. Remove these Console.WriteLine statements as they appear to be temporary debugging additions.
| Console.WriteLine("Error"); | |
| Console.WriteLine(ex); |
| return new TenantsResponse | ||
| { | ||
| TenantName = t.TenantName, | ||
| AdminConnectrionString = new EdfiConnectrionString() |
There was a problem hiding this comment.
Property name has typos: 'AdminConnectrionString' should be 'AdminConnectionString'.
| host = adminHostAndDatabase.Host, | ||
| database = adminHostAndDatabase.Database | ||
| }, | ||
| SecurityConnectrionString = new EdfiConnectrionString() |
There was a problem hiding this comment.
Property name has typos: 'SecurityConnectrionString' should be 'SecurityConnectionString'.
| return Results.Ok(new TenantsResponse | ||
| { | ||
| TenantName = tenant.TenantName, | ||
| AdminConnectrionString = new EdfiConnectrionString() |
There was a problem hiding this comment.
Property name has typos: 'AdminConnectrionString' should be 'AdminConnectionString'.
| host = adminHostAndDatabase.Host, | ||
| database = adminHostAndDatabase.Database | ||
| }, | ||
| SecurityConnectrionString = new EdfiConnectrionString() |
There was a problem hiding this comment.
Property name has typos: 'SecurityConnectrionString' should be 'SecurityConnectionString'.
| public EdfiConnectrionString? SecurityConnectrionString { get; set; } | ||
| } | ||
|
|
||
| public class EdfiConnectrionString |
There was a problem hiding this comment.
Class name has typo: 'EdfiConnectrionString' should be 'EdfiConnectionString'.
| var response = new TenantsResponse | ||
| { | ||
| TenantName = defaultTenant.TenantName, | ||
| AdminConnectrionString = new EdfiConnectrionString() |
There was a problem hiding this comment.
Property name has typos: 'AdminConnectrionString' should be 'AdminConnectionString'.
| host = adminHostAndDatabase.Host, | ||
| database = adminHostAndDatabase.Database | ||
| }, | ||
| SecurityConnectrionString = new EdfiConnectrionString() |
There was a problem hiding this comment.
Property name has typos: 'SecurityConnectrionString' should be 'SecurityConnectionString'.
| public EdfiConnectrionString? AdminConnectrionString { get; set; } | ||
| public EdfiConnectrionString? SecurityConnectrionString { get; set; } |
There was a problem hiding this comment.
Class and property names have typos: 'EdfiConnectrionString' should be 'EdfiConnectionString', 'AdminConnectrionString' should be 'AdminConnectionString', and 'SecurityConnectrionString' should be 'SecurityConnectionString'.
| public EdfiConnectrionString? SecurityConnectrionString { get; set; } | ||
| } | ||
|
|
||
| public class EdfiConnectrionString |
There was a problem hiding this comment.
Class name has typo: 'EdfiConnectrionString' should be 'EdfiConnectionString'.
DavidJGapCR
force-pushed
the
ADMINAPI-1297
branch
from
f4b6415 to
11e720a
Compare
DavidJGapCR
changed the title
| var json = _fileProvider.ReadAllText(); | ||
| try | ||
| { | ||
| var root = JsonNode.Parse(json) ?? throw new InvalidOperationException("appsettings.json is empty or invalid."); |
There was a problem hiding this comment.
@DavidJGapCR please revert this change. We cannot let the application rewrite the settings file.
This reverts commit f9a07fc.
4 participants
No description provided.