woj-tek commented on PR #1559: URL: https://github.com/apache/james-project/pull/1559#issuecomment-1545962554
> We are missing documentations for the new webadmin endpoints: I'll add it later on, though would be nice to clarify/confirm endpoints - do we want to have it directly or under `/server/<protocol>? > I would also say that we lack some tests. Did you think of a potential testing strategy for this changeset? I think the most crucial would be testing the reload itself. I see that in `protocols-library` there are already tests using resources keystore - one possibility would be to have second keystore, update `MemoryFileSystem` with the new one, initiate certificate reload and check the result. Another opportunity would be generating certificates on the fly, but it's more difficult with newer JDK versions it's difficult. Testing whole path (rest->server->protocol connection) could be slightly more problematic as it would require certain duplication to cover all protocols (and I'm not sure that it's needed). > Also I am unsure the feature of "reloading SSL" triggered per protocol is truely useful. It could be (possiby having different certificates for different protocols) but I would say it's the edge case, however... > I would rather propose a single endpoint that triggers the reload on all relevant servers: something like `POST /servers?reloadSSLCertificate`. This can be achieved with a multibinder on `AbstractServerFactory`. > > Finally keeping some orthogonality across components, and refraining from adding the webadmin dependency to the protcol-library, seems necessary to me. I am ready to provide help if need be to help sort out this issue. ...I initially had that in mind and asked about dedicated module, to handle it, to which you replied with "Each protocol module can independently define the bindings without needing a centralized place at all..." hence I followed your guidance :-) TBH, in our downstream project I used that approach and simply rely on injected `Set<AbstractServerFactory>` to reload certificates for all servers/protocols :-) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
