Capabilities endopoint#17
Conversation
|
Without the tests section, I am still working on it |
ronenkapelian
left a comment
ronenkapelian
left a comment
There was a problem hiding this comment.
- missing test configuration files:
boilerplate - middleware directory should called :"middlewares"
3.Stale Jest devDependencies in package.json:
eslint-plugin-jest
ts-jest
@types/multer
Hardcoded production cluster URLs in config/default.json
"manager": {
"url": "https://auth-manager-infra-services.apps.j1lk3njp.eastus.aroapp.io"
},
"opa": {
"servers": {
"prod": "http://opa-dev-opa-service.infra-services.svc.cluster.local:8181",
...
}
}
default.json is committed to git and is the fallback for all environments. Committing real internal cluster addresses and hostnames here exposes infrastructure topology in version control. default.json should contain safe placeholder values (e.g., http://localhost:8080). Actual URLs belong in environment-specific config or Kubernetes ConfigMaps.
5.openapi
The spec defines:
components:
schemas:
error:
type: object
required: [message]
properties:
message: { type: string }
But the error schema is never $ref'd in any response. The /capabilities endpoint only defines 200. Error responses (e.g., 500) should reference this schema so API clients know what error payloads look like.
please fix all openapi lint issues, use also AI and documentation to review the openapi and validate it correct, define all needed data and hermetic represent the server
MaxListenersExceededWarning in integration tests
Running the integration suite produces:
MaxListenersExceededWarning: Possible EventEmitter memory leak detected.
11 exit listeners added to [process].
This happens because getApp() is called in beforeEach, which calls createTerminus, which registers onSignal exit handlers. Each iteration adds another listener. Use beforeAll for app initialisation in integration tests to create the app once per suite, not once per test.
7.jsonwebtoken and jwks-rsa in production dependencies
These packages are listed in dependencies (not devDependencies) but are not imported anywhere in the codebase. They appear to be pre-emptively installed for the future auth phase mentioned in the architectural plan.
Until that work starts they add unnecessary weight to the production image. Consider removing them and adding them back when the auth middleware is implemented.
8.interfaces.ts with iconfig not in use, remove.
9.please edit the description section of the pr on github, summerized what you done on this pr, what include, according to the design you got
There was a problem hiding this comment.
it seems its dead code and not used (shouldnt be used with our config package and server)
| }; | ||
|
|
||
| // Placeholder — will validate JWTs in the future auth phase | ||
| export const authMiddleware = (req: Request, res: Response, next: NextFunction): void => { |
There was a problem hiding this comment.
don't understand the purpose, also its function is identical to opa middleware, move it to middlewares directory and export from them one single function
| public getCapabilities(): CapabilitiesResponse { | ||
| this.logger.info({ msg: 'getting capabilities' }); | ||
|
|
||
| const config = getConfig(); |
There was a problem hiding this comment.
The boilerplate standard is to inject SERVICES.CONFIG via the constructor, just as ServerBuilder does. Calling a singleton getter inside a method makes the class harder to test (forces vi.mock('@common/config') globally) and defeats the purpose of using tsyringe
SEE JOBNIK, AND CONFIG SERVER FOR EXAMPLE
There was a problem hiding this comment.
managerRouter.ts and opaRouter.ts are registered as useFactory in the DI container and receive a dependencyContainer parameter, but both call getConfig() directly instead of resolving from the container:
const managerRouterFactory: FactoryFunction = () => { // ← container not used
const config = getConfig(); // ← bypasses DI
The middleware functions they call (managerEnabledMiddleware, opaEnabledMiddleware, etc.) also call getConfig() directly. This is inconsistent with the pattern used in ServerBuilder and makes the wiring invisible to the container.
| describe('Bad Path', function () { | ||
| it('should in theory test 400 status code', function () { | ||
| expect(true).toBe(true); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Sad Path', function () { | ||
| it('should in theory test 500 status code', function () { | ||
| expect(true).toBe(true); |
There was a problem hiding this comment.
dont use placeholder, implement relevant tests
2 participants
Related issues: #XXX , #XXX ...
Closes #XXX ...
Further information: