EFIAL-458-config/dependencies and EFIAL-459-partner-sync#76
EFIAL-458-config/dependencies and EFIAL-459-partner-sync#76rtavernaea wants to merge 35 commits into
Conversation
edandylytics
left a comment
There was a problem hiding this comment.
Still taking it in and need to try it out. Overall, it's looking pretty good -- just a few early thoughts in the comments.
One scope update; let's hold off on including any of the TX config or other components until we're building that piece. The enum value is fine, but it's hard to tell whether the rest is correct until we're actually building the TX sync
| const partnerIdsToDelete: string[] = []; | ||
| const partnerIdsToUndelete: string[] = []; | ||
|
|
||
| if (partnersResult.status === 'success') { |
There was a problem hiding this comment.
I think we should stop early if the partner request fails. Continuing to sync tenants is probably fine, but I find it difficult to reason about its correctness.
| } | ||
|
|
||
| for (const partner of existingPartners) { | ||
| if (!!partner.managedBy && !partner.deletedOn && !apiPartnerCodes.has(partner.id)) { |
There was a problem hiding this comment.
Where do we check that the partner we're deleting is managed by this specific sync? E.g. TX doesn't delete UM partners and vice verse?
There was a problem hiding this comment.
Oh right, I wasn't thinking of that but I guess we need it because there is a tx partner in UM?
There was a problem hiding this comment.
Yep -- TX being in the UM results is a clearer case than the one I mentioned. Here's how I think about it: It's Runway's job to determine which partners each sync manages. Runway shouldn't trust external systems to say which partners it can update. That means we need to ensure that it's Runway's internal state (partner.magagedBy) that scopes the sync, not what the external systems report.
| return; | ||
| } | ||
|
|
||
| const tenantMap: Map<string, GetTenantDto> = tenantsByPartner.get(partnerId) ?? new Map(); |
There was a problem hiding this comment.
Can this be Tenant instead of GetTenantDto? Looks like it holds tenants from Prisma, not tenant DTOs. They're mostly the same shape today, but we could update the DTO in a way that'd break this
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
edandylytics
left a comment
There was a problem hiding this comment.
A couple more comments.
| const url = this.get('UM_URL'); | ||
| const auth0Domain = this.get('UM_AUTH0_DOMAIN'); | ||
| const clientId = this.get('UM_CLIENT_ID'); | ||
| const clientSecret = this.get('UM_CLIENT_SECRET'); |
There was a problem hiding this comment.
Deployed environments will need to look up credentials in an AWS secret. You can check out other methods on this service for how to use a secret and fall back to env vars if a secret doesn't exist. postgresPoolConfig is a good example.
| } | ||
|
|
||
| for (const partner of existingPartners) { | ||
| if (!!partner.managedBy && !partner.deletedOn && !apiPartnerCodes.has(partner.id)) { |
There was a problem hiding this comment.
Yep -- TX being in the UM results is a clearer case than the one I mentioned. Here's how I think about it: It's Runway's job to determine which partners each sync manages. Runway shouldn't trust external systems to say which partners it can update. That means we need to ensure that it's Runway's internal state (partner.magagedBy) that scopes the sync, not what the external systems report.
There was a problem hiding this comment.
I think we should probably refactor away from this coordinator class. It looks like it does two things:
- Initialize PgBoss and manage its lifecycle
- Start up the sync
For PgBoss, I expect we'll be using that in other places in the app soon and it'd be helpful to have that as it's own injectable component that can be shared.
Once you remove the PgBoss initialization, do we need a coordinator? It seems each sync service could start itself in its own OnModuleInit. Then we don't need the getSyncConfig wrapper either.
Let me know if I'm missing something there, though.
This PR addresses EDFIAL-458 and EDFIAL-459.
EDFIAL-458:
TX_SYNC_CRONSYNC_CRONvalue but kept them separate for nowPartnerSyncCoordinatorhandles scheduling and kicking off each sync based on what config values are available.EDFIAL-459:
Both syncs share the same pg-boss connection
user_management_syncsync_managed: null) are left alone by the synctx_sync