-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove app-deployment file #344
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
import { removeAppConfig } from '~/src/helpers/remove/workflows/remove-app-config.js' | ||
import { removeDashboard } from '~/src/helpers/remove/workflows/remove-dashboard.js' | ||
import { removeNginxUpstreams } from '~/src/helpers/remove/workflows/remove-nginx-upstreams.js' | ||
import { | ||
removeAppConfig, | ||
removeDashboard, | ||
removeNginxUpstreams, | ||
removeSquidConfig, | ||
removeTenantInfrastructure | ||
} from '~/src/helpers/remove/workflows/index.js' | ||
|
@@ -18,22 +18,21 @@ async function triggerRemoveWorkflows(serviceName, repository, logger) { | |
logger.info(`Triggering remove workflows service: ${serviceName}`) | ||
const isTestSuite = repository.topics.includes('test-suite') | ||
|
||
if (isFeatureEnabled(featureToggles.removeServiceWorkflows)) { | ||
if (!isTestSuite) { | ||
const zone = repository.topics.includes('backend') | ||
? 'Protected' | ||
: 'Public' | ||
if (!isFeatureEnabled(featureToggles.removeServiceWorkflows)) { | ||
logger.info('Remove service workflows feature is disabled') | ||
return | ||
} | ||
|
||
await removeDashboard(serviceName, logger) | ||
await removeNginxUpstreams(serviceName, zone, logger) | ||
} | ||
if (!isTestSuite) { | ||
const zone = repository.topics.includes('backend') ? 'Protected' : 'Public' | ||
|
||
await removeAppConfig(serviceName, logger) | ||
await removeSquidConfig(serviceName, logger) | ||
await removeTenantInfrastructure(serviceName, logger) | ||
} else { | ||
logger.info('Remove service workflows feature is disabled') | ||
await removeDashboard(serviceName, logger) | ||
await removeNginxUpstreams(serviceName, zone, logger) | ||
} | ||
|
||
await removeAppConfig(serviceName, logger) | ||
await removeSquidConfig(serviceName, logger) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🏎️ isn't always better 😅 |
||
await removeTenantInfrastructure(serviceName, logger) | ||
} | ||
|
||
export { triggerRemoveWorkflows } |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
import { triggerRemoveWorkflows } from './trigger-remove-workflows.js' | ||
import { | ||
removeAppConfig, | ||
removeDashboard, | ||
removeNginxUpstreams, | ||
removeSquidConfig, | ||
removeTenantInfrastructure | ||
} from '~/src/helpers/remove/workflows/index.js' | ||
import { isFeatureEnabled } from '~/src/helpers/feature-toggle/is-feature-enabled.js' | ||
|
||
jest.mock('~/src/helpers/remove/workflows') | ||
jest.mock('~/src/helpers/feature-toggle/is-feature-enabled') | ||
|
||
const logger = { | ||
info: jest.fn() | ||
} | ||
|
||
describe('#triggerRemoveWorkflows', () => { | ||
const serviceName = 'some-service' | ||
const backendRepository = { | ||
topics: ['backend'] | ||
} | ||
const testRepository = { | ||
topics: ['test-suite'] | ||
} | ||
|
||
isFeatureEnabled.mockReturnValue(true) | ||
|
||
test('Should trigger remove app config workflow', async () => { | ||
await triggerRemoveWorkflows(serviceName, backendRepository, logger) | ||
|
||
expect(removeAppConfig).toHaveBeenCalledWith(serviceName, logger) | ||
}) | ||
|
||
test('Should trigger remove squid config workflow', async () => { | ||
await triggerRemoveWorkflows(serviceName, backendRepository, logger) | ||
|
||
expect(removeSquidConfig).toHaveBeenCalledWith(serviceName, logger) | ||
}) | ||
|
||
test('Should trigger remove tenant infrastructure workflow', async () => { | ||
await triggerRemoveWorkflows(serviceName, backendRepository, logger) | ||
|
||
expect(removeTenantInfrastructure).toHaveBeenCalledWith(serviceName, logger) | ||
}) | ||
|
||
test('Should trigger remove dashboard workflow if not test suite', async () => { | ||
await triggerRemoveWorkflows(serviceName, backendRepository, logger) | ||
|
||
expect(removeDashboard).toHaveBeenCalledWith(serviceName, logger) | ||
}) | ||
|
||
test('Should not trigger remove dashboard workflow if test suite', async () => { | ||
await triggerRemoveWorkflows(serviceName, testRepository, logger) | ||
|
||
expect(removeDashboard).toHaveBeenCalledTimes(0) | ||
}) | ||
|
||
test('Should trigger remove nginx upstreams workflow if not test suite', async () => { | ||
await triggerRemoveWorkflows(serviceName, backendRepository, logger) | ||
|
||
expect(removeNginxUpstreams).toHaveBeenCalledWith( | ||
serviceName, | ||
'Protected', | ||
logger | ||
) | ||
}) | ||
|
||
test('Should not trigger remove nginx upstreams workflow if test suite', async () => { | ||
await triggerRemoveWorkflows(serviceName, testRepository, logger) | ||
|
||
expect(removeNginxUpstreams).toHaveBeenCalledTimes(0) | ||
}) | ||
|
||
test('Should log if remove service workflows feature is disabled', async () => { | ||
isFeatureEnabled.mockReturnValue(false) | ||
|
||
await triggerRemoveWorkflows(serviceName, backendRepository, logger) | ||
|
||
expect(removeTenantInfrastructure).toHaveBeenCalledTimes(0) | ||
expect(logger.info).toHaveBeenCalledWith( | ||
'Remove service workflows feature is disabled' | ||
) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import Joi from 'joi' | ||
|
||
import { deleteDeploymentFiles } from '~/src/api/undeploy/helpers/delete-deployment-file.js' | ||
|
||
export const deleteDeploymentFilesController = { | ||
options: { | ||
auth: { | ||
strategy: 'azure-oidc', | ||
access: { | ||
scope: ['admin'] | ||
} | ||
}, | ||
validate: { | ||
params: Joi.object({ | ||
serviceName: Joi.string().min(1).required() | ||
}) | ||
} | ||
}, | ||
handler: async (request, h) => { | ||
const { serviceName, environment } = request.params | ||
|
||
await deleteDeploymentFiles({ | ||
serviceName, | ||
environment, | ||
logger: request.logger | ||
}) | ||
return h.response().code(204) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import { createLogger } from '~/src/helpers/logging/logger.js' | ||
import { featureToggles } from '~/src/helpers/feature-toggle/feature-toggles.js' | ||
import { isFeatureEnabled } from '~/src/helpers/feature-toggle/is-feature-enabled.js' | ||
import { removeDeployment } from '~/src/helpers/remove/workflows/remove-deployment.js' | ||
|
||
/** | ||
* @param {{serviceName: string, logger: [import('pino').Logger]}} options | ||
*/ | ||
export async function deleteDeploymentFiles({ | ||
serviceName, | ||
logger = createLogger() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
}) { | ||
logger.info(`Deleting deployment files for ${serviceName}`) | ||
|
||
if (!isFeatureEnabled(featureToggles.undeploy.deleteDeploymentFiles)) { | ||
logger.info('Deleting deployment file feature is disabled') | ||
return | ||
} | ||
|
||
await removeDeployment(serviceName, logger) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
import { featureToggles } from '~/src/helpers/feature-toggle/feature-toggles.js' | ||
import { isFeatureEnabled } from '~/src/helpers/feature-toggle/is-feature-enabled.js' | ||
import { removeDeployment } from '~/src/helpers/remove/workflows/remove-deployment.js' | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is a simple function and it's just testing if the feature toggle is respected. That is the only branching. Overriding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now granted there are other functions that mock too much but not this one There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the link. I will try and use that in the future. Did not think of |
||
|
||
const serviceName = 'some-service' | ||
const logger = { info: jest.fn() } | ||
|
||
describe('#deleteDeploymentFiles', () => { | ||
test('should delete all deployment files for a service', async () => { | ||
isFeatureEnabled.mockReturnValue(true) | ||
|
||
await deleteDeploymentFiles({ serviceName, logger }) | ||
|
||
expect(isFeatureEnabled).toHaveBeenCalledWith( | ||
featureToggles.undeploy.deleteDeploymentFiles | ||
) | ||
expect(removeDeployment).toHaveBeenCalledWith(serviceName, logger) | ||
}) | ||
|
||
test('should not delete deployment file if feature disabled', async () => { | ||
isFeatureEnabled.mockReturnValue(false) | ||
|
||
await deleteDeploymentFiles({ serviceName, logger }) | ||
|
||
expect(removeDeployment).toHaveBeenCalledTimes(0) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Promise.all()
orPromise.allSettled()
would mean these can become parallel