[Synthetics] Refactor: Create monitor configs repository !! #202325
Conversation
botelastic
bot
added
ci:project-deploy-observability
|
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
shahzad31
added
release_note:skip
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]
History
|
| this.monitorConfigRepository = new MonitorConfigRepository( | ||
| savedObjectsClient, | ||
| server.encryptedSavedObjects.getClient() | ||
| ); |
There was a problem hiding this comment.
Shouldn't we inject the whole dependency instead of instantiating it in the constructor?
But it's already a good thing to introduce a repository pattern 👍🏻
| expect( | ||
| await route.handler({ | ||
| // @ts-expect-error partial implementation for testing | ||
| request: { query: {} }, | ||
| // @ts-expect-error partial implementation for testing | ||
| syntheticsEsClient: jest.fn(), | ||
| savedObjectClient: jest.fn(), | ||
| // @ts-expect-error partial implementation for testing | ||
| monitorConfigRepository: { getAll }, |
There was a problem hiding this comment.
If we create a mockMonitorConfigRepository somewhere that implements the interface with jest.fn() we can get ride of the @ts directive and reuse it in the other tests
| soClient: SavedObjectsClientContract; | ||
| encryptedSavedObjectsClient: EncryptedSavedObjectsClient; | ||
| constructor( | ||
| soClient: SavedObjectsClientContract, | ||
| encryptedSavedObjectsClient: EncryptedSavedObjectsClient | ||
| ) { | ||
| this.soClient = soClient; | ||
| this.encryptedSavedObjectsClient = encryptedSavedObjectsClient; | ||
| } |
There was a problem hiding this comment.
neat: Use short hand constructor to remove boilerplate
| soClient: SavedObjectsClientContract; | |
| encryptedSavedObjectsClient: EncryptedSavedObjectsClient; | |
| constructor( | |
| soClient: SavedObjectsClientContract, | |
| encryptedSavedObjectsClient: EncryptedSavedObjectsClient | |
| ) { | |
| this.soClient = soClient; | |
| this.encryptedSavedObjectsClient = encryptedSavedObjectsClient; | |
| } | |
| constructor( | |
| private soClient: SavedObjectsClientContract, | |
| private encryptedSavedObjectsClient: EncryptedSavedObjectsClient | |
| ) { } |
| get = async (id: string) => { | ||
| return await this.soClient.get<EncryptedSyntheticsMonitorAttributes>(syntheticsMonitorType, id); | ||
| }; | ||
|
|
||
| async getDecrypted(id: string, spaceId: string): Promise<SavedObject<SyntheticsMonitor>> { |
There was a problem hiding this comment.
Let's not used arrow function for class methods, it comes with some unnecessary memory overhead and in our case we don't need it.
Plus it makes this class more consistent if we pick just one way of defining methods
| const monitorConfigRepository = new MonitorConfigRepository( | ||
| soClient, | ||
| encryptedSavedObjects.getClient() |
There was a problem hiding this comment.
Isn't it possible to instantiate the SyntheticsMonitorClient with the MonitorConfigRepository directly? Maybe that's too much of a refactoring. If we were to move the SyntheticsMonitorClient into the SynthethicsRouterWrapper were we instantiate the repository, we could directly inject it as a dependency?
Also, maybe not possible to do if the SyntheticsMonitorClient is used outside of the route, I haven't checked deeper
| ); | ||
| } | ||
|
|
||
| getAll = async ({ |
|
🤖 Jobs for this PR can be triggered through checkboxes. 🚧
ℹ️ To trigger the CI, please tick the checkbox below 👇
|
Summary
Create monitor configs repository around monitor saved object to make sure all operations are performed from same class.
This will be helpful when we create a new saved object to support multiple-spaces !!
Testing
All unit tests, api tests passing should be more than enough !!