Remove app-deployment file #344
Conversation
15f2c21 to
d31ea99
Compare
src/api/decommission-service/helpers/trigger-remove-workflows.js
Outdated
Show resolved
Hide resolved
8faa7e3 to
ab1fb57
Compare
| */ | ||
| export async function deleteDeploymentFiles({ | ||
| serviceName, | ||
| logger = createLogger() |
There was a problem hiding this comment.
Is this is being passed in for ease of testing? Why not just mock the logger in tests? Then you don't have "test code" in production code. Which I'm not a massive fan of
There was a problem hiding this comment.
If I don't pass the logger the statements from different async calls tend to be out of sync and harder to trace the journey. Also this seems a lot tidier.
There was a problem hiding this comment.
Interesting. Thats a bit of a smell and I'd say there is something else going on there if async logs are not showing in order and you have to pass the logger around to make them. Might need some more investigation as to the cause of this issue?
| import { deleteDeploymentFiles } from '~/src/api/undeploy/helpers/delete-deployment-file.js' | ||
|
|
||
| jest.mock('~/src/helpers/feature-toggle/is-feature-enabled') | ||
| jest.mock('~/src/helpers/remove/workflows/remove-deployment') |
There was a problem hiding this comment.
Quite a lot of mocking going on here. Looks like this is just testing inputs and outputs. Lots of mocks in tests for me is a smell. The code can change, the tests stay mocked and green. Real code being tested is being missed. Can the mocks be removed? Or go down to a lower level? Can the http calls be mocked with nock?
There was a problem hiding this comment.
It is a simple function and it's just testing if the feature toggle is respected. That is the only branching.
Overriding process.env.??? was not really working and looked messy and I don't really want to be testing how config works in this unit test.
There was a problem hiding this comment.
Now granted there are other functions that mock too much but not this one
There was a problem hiding this comment.
Overriding process.env.??? was not really working and looked messy and I don't really want to be testing how config works in this unit test.
You can mock out config quite nicely in tests. If needed. Theres examples around https://github.com/DEFRA/cdp-user-service-backend/blob/6c57e6e7b9460f0d55c27fe383ae9877674aae4d/src/helpers/proxy.test.js#L17-L25
There was a problem hiding this comment.
Thanks for the link. I will try and use that in the future. Did not think of config.set as an option
| } | ||
|
|
||
| await removeAppConfig(serviceName, logger) |
There was a problem hiding this comment.
Promise.all() or Promise.allSettled() would mean these can become parallel
There was a problem hiding this comment.
Could do, but there is no need as this has no performance or concurrency requirements. In fact, as sequential as possible is preferred.
There was a problem hiding this comment.
🏎️ isn't always better 😅
| await removeTenantInfrastructure(serviceName, logger) | ||
| } else { | ||
| logger.info('Remove service workflows feature is disabled') | ||
| await removeDashboard(serviceName, logger) |
There was a problem hiding this comment.
Promise.all() or Promise.allSettled() would mean these can become parallel
Calls new remove service from cdp-app-deployments