feat: support a few domains (registry) #280
Conversation
e1dae5b to
1f7a734
Compare
1f7a734 to
337fa88
Compare
| next: boolean, | ||
| templateName: string, | ||
| } | ||
| // now it's useless |
| const getAllRouterDomains = async (req: Request, res: Response): Promise<void> => { | ||
| const routerDomains = await db.select().from('router_domains'); | ||
|
|
||
| res.setHeader('Content-Range', routerDomains.length); //Stub for future pagination capabilities |
There was a problem hiding this comment.
Let's add pagination from very beginning. It's much easier to do it now.
registry/server/routes/config.ts
Outdated
| @@ -25,7 +26,7 @@ router.get('/', async (req, res) => { | |||
| routes: [] as any[], | |||
| specialRoutes: {}, | |||
| settings: {}, | |||
| routerDomains: {}, | |||
| routerDomains, | |||
There was a problem hiding this comment.
Hmm, what if instead of adding routerDomains property to config - we would turn domainId: int field into domain: string | null for respective routes?
There was a problem hiding this comment.
on the one hand, I think it's good idea, but on the other hand it will be a little bit harder to detect inside ILC - "do we respect the current domain".
| @@ -6,3 +6,4 @@ export { default as sharedProps } from '../sharedProps/routes'; | |||
| export { default as authEntities } from '../authEntities/routes'; | |||
| export { default as settings } from '../settings/routes'; | |||
| export { default as versioning } from '../versioning/routes'; | |||
| export { default as routerDomains } from '../routerDomains/routes'; | |||
There was a problem hiding this comment.
I think you also need to add filtration capability by domainId to getRoutes API
There was a problem hiding this comment.
sorry, can't understand, what do you mean?
There was a problem hiding this comment.
I want to be able to filter routes list by domainId, in routes API
registry/tests/appRoutes.spec.ts
Outdated
| domainId, | ||
| }; | ||
|
|
||
| let response = await request.post(example.url) |
There was a problem hiding this comment.
this should be within another try/finally block
registry/tests/appRoutes.spec.ts
Outdated
| expect(response.body).deep.equal(expectedRoute); | ||
| } finally { | ||
| await request.delete(example.url + id); | ||
| await request.delete(example.routerDomain.url + domainId).expect(204); |
There was a problem hiding this comment.
rm expect here, it's not necessary
There was a problem hiding this comment.
but what if delete is broken? in this case we will face the problem with the next it, instead of current
There was a problem hiding this comment.
We should have an explicit test case for delete operation instead of this implicit check, IMO
registry/tests/appRoutes.spec.ts
Outdated
| @@ -469,6 +556,32 @@ describe(`Tests ${example.url}`, () => { | |||
| } | |||
| }); | |||
|
|
|||
| it('should successfully update record with existed domainId', async () => { | |||
| const domainId1 = await createRouterDomain(example.routerDomain); | |||
There was a problem hiding this comment.
Same as above, you need to do proper cleanup in case of error
registry/tests/routerDomains.spec.ts
Outdated
| }); | ||
|
|
||
| it('should successfully create record', async () => { | ||
| const responseCreation = await request.post(example.url) |
There was a problem hiding this comment.
ensure cleanup in case of test failure with try/finally. Here and in all tests below
registry/tests/appRoutes.spec.ts
Outdated
| @@ -412,6 +478,30 @@ describe(`Tests ${example.url}`, () => { | |||
| } | |||
| }); | |||
|
|
|||
| it('should not update record with non-existing domainId', async () => { | |||
| const domainId = await createRouterDomain(example.routerDomain); | |||
There was a problem hiding this comment.
I still don't see cleanup code for this
registry/tests/appRoutes.spec.ts
Outdated
| }); | ||
|
|
||
| it('should create record with existed domainId', async () => { | ||
| const domainId = await createRouterDomain(example.routerDomain); |
There was a problem hiding this comment.
Same here, I assume we need to cleanup this too
…er routes from config API
No description provided.