feat(cleanup): add bulk resource deletion script for integration tests (DEVX-433) #815

evansinho · 2024-10-04T15:22:53Z

This PR introduces a script to handle the bulk deletion of various resources, ensuring the system stays clean and within resource limits. Key features include:

Unpublishing and deleting all products.
Safe deletion of resources, skipping any referenced by others (e.g., tax categories, customer groups, stores).
Improved error handling and logging for easier tracking.
Resolved versioning conflicts during resource updates to prevent concurrency issues.
This will help maintain a cleaner environment and avoid hitting commercetools resource limits.

…EVX-433) - cleanup all resources Refs DEVX-433

changeset-bot · 2024-10-04T15:22:56Z

⚠️ No Changeset found

Latest commit: ba6f8f2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

cneijenhuis · 2024-10-07T08:14:59Z

packages/platform-sdk/test/integration-tests/CleanEverythingIntegrationTest.ts

+    )
+    console.log(`Deleted ${productTypes.body.results.length} Product Types`)
+
+    // Delete all standalone products (not tied to a product type)


Sorry, can you clarify this point - I thought products need always to be tied to a product type? https://docs.commercetools.com/api/projects/products#product

@cneijenhuis Thanks for pointing that out! Yes, you're right, products are always tied to a product type. However, when deleting a product, we don't need to reference the product type explicitly in the deletion process. In this case, I'm fetching all the products and deleting them directly. The link to the product type is part of the product’s metadata, but it doesn’t need to be called out specifically for the deletion to happen. I focus on unpublishing and then deleting the product itself.

Let me know if that makes sense!

cneijenhuis · 2024-10-07T08:24:50Z

packages/platform-sdk/test/integration-tests/CleanEverythingIntegrationTest.ts

+          taxCategory.id,
+          taxCategory.version,
+          async () => {
+            const productsReferencingTaxCategory = await apiRoot


I'm not following this for several reasons:

At this point, we should have already deleted all products - why should we double-check here?

Why are you trying to duplicate the "safe deletion" logic here? If it isn't safe to delete, the API should refuse to delete (and you should react to the error). E.g. for tax categories, shipping methods also reference them, and the API will refuse the request. Isn't it better to react to that error?

You're right that by the time we get to tax categories, we should have already deleted all products, and ideally, there wouldn't be any remaining references. The reason I added that check was to be extra cautious in case anything slipped through or there were edge cases where products might still reference a tax category. It’s more of a precaution to prevent unexpected errors during the cleanup.

As for duplicating the "safe deletion" logic, I agree...it’s better to rely on the API’s error handling when trying to delete something that’s still referenced, like in the case of shipping methods referencing tax categories. I can adjust the code to simply handle those errors directly rather than pre-checking, which will make the process cleaner and more efficient.

@evansinho I would recommend to delete straightaway and not checking.
Reasons:

At this point products are all delete. If not, then it should fail and you should investigate why. It could be an error in the script and then we have to fix it. If it's a platform error, then we should report it.

Checking this requires extra request/response. This script could be used as a reference for customers, so we should avoid doing unnecessary actions.

Please check also for other resources that use safeDeleteResource().

ajimae · 2024-10-10T10:29:26Z

Nice job 👍🏽

I was looking at the jira ticket and I could see that other methods were also hinted. I think that aside this standalone script solution, we should also have in addition test hooks for relevant tests suites such as a beforeAll and afterAll hooks that does this just as we have here

We created the resource we needed for the test and immediately delete the resource(s) after the tests finished running. This way we are always sure of a clean slate anytime we are running the integration tests.

What do you think?

codecov · 2024-10-10T10:36:06Z

Codecov Report

Attention: Patch coverage is 0% with 88 lines in your changes missing coverage. Please review.

Project coverage is 71.35%. Comparing base (1f26569) to head (ba6f8f2).
Report is 10 commits behind head on master.

Files with missing lines	Patch %	Lines
...ntegration-tests/CleanEverythingIntegrationTest.ts	0.00%	88 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #815       +/-   ##
===========================================
- Coverage   93.07%   71.35%   -21.73%     
===========================================
  Files          25       26        +1     
  Lines         289      377       +88     
  Branches       14       17        +3     
===========================================
  Hits          269      269               
- Misses         20      108       +88

Flag	Coverage Δ
integrationtests	`71.35% <0.00%> (-21.73%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

evansinho · 2024-10-10T11:54:43Z

@ajimae Thanks for the feedback! 👍🏽 I completely agree. Having the beforeAll and afterAll hooks for the test suites to handle resource creation and deletion will ensure a clean slate for each test run, making the process more robust. I'll create a new branch and start implementing this approach alongside the standalone script solution.

lojzatran

Good job 🔥

Since this is part of our SDK and we might later reference our customers how to do the proper clean up, I added some comments how to improve the script in general.

lojzatran · 2024-10-10T19:40:23Z

packages/platform-sdk/test/integration-tests/CleanEverythingIntegrationTest.ts

+      .withId({ ID: resourceId })
+      .delete({ queryArgs: { version } })
+      .execute()
+    console.log(`Deleted ${resourceType.slice(0, -1)} with ID: ${resourceId}`)


Since the resource is deleted, you cannot do anything with the ID outputted here, so I would recommend removing this log.

Rather than this log it's more useful to output a summary in the end of the script - which resources were deleted and how many successfully and how many errors (maybe how long). Also output some kind of counting for long running deletion process - e.g. every 100 resources output Deleted ${count} ${resourceType.slice(0, -1)}. This way user has a way to follow the execution.

In general we should minimize logging things you cannot act upon - if it's info that sth is successfully deleted, just log the summary in the end of the whole process. On the other hand, the errors should be logged immediately as you have it here. Like this you could see the errors immediately and they're not lost in many logs.

lojzatran · 2024-10-10T19:41:08Z

packages/platform-sdk/test/integration-tests/CleanEverythingIntegrationTest.ts

+// Function to unpublish and delete a product
+const unpublishAndDeleteProduct = async (product: Product) => {
+  if (product.masterData.published) {
+    console.log(`Unpublishing product: ${product.id}`)


Remove this and write instead a one-line summary in the end of the deletion process like I suggested above.

lojzatran · 2024-10-10T19:47:03Z

packages/platform-sdk/test/integration-tests/CleanEverythingIntegrationTest.ts

+        await deleteProductsForProductType(productType.id)
+      })
+    )
+    console.log(`Deleted ${productTypes.body.results.length} Product Types`)


This log is not correct. In the lines above only products were deleted, product types were not deleted.
Actually product types are not deleted anywhere.

lojzatran · 2024-10-10T19:49:18Z

packages/platform-sdk/test/integration-tests/CleanEverythingIntegrationTest.ts

+    const productTypes = await apiRoot.productTypes().get().execute()
+    await Promise.all(
+      productTypes.body.results.map(async (productType) => {
+        await deleteProductsForProductType(productType.id)


It's not necessary to fetch products per product type if we're going to delete all products. Just fetch all products without where query and delete. This could save some requests/responses in case there are less products.

lojzatran · 2024-10-10T19:55:58Z

packages/platform-sdk/test/integration-tests/CleanEverythingIntegrationTest.ts

+
+// Function to delete all products in the system
+const deleteAllProducts = async () => {
+  const products = await apiRoot.products().get().execute()


This won't work if there are more than 20 (default limit number) products. You need to use process() on the client to iterate through all the products. (

commercetools-sdk-typescript/packages/sdk-client-v3/src/client/client.ts

Line 38 in 511ba9a

export function process(

, https://docs.commercetools.com/api/performance-tips - last bullet point). This applies to all other resources as well.

Additionally raise the limit to maximum 500 so there are less requests/responses in total (

commercetools-sdk-typescript/packages/sdk-client-v3/src/client/client.ts

Line 69 in 511ba9a

limit: 20,

)

lojzatran · 2024-10-10T20:06:23Z

packages/platform-sdk/test/integration-tests/CleanEverythingIntegrationTest.ts

+  console.log(
+    `Found ${products.body.results.length} products for product type ${productTypeId}...`
+  )
+  return Promise.all(


It's not recommended to use Promise.all with requests to commercetools. Rather use Promise.map and limit the concurrency to some lower number like 20. The reason is CTP cannot handle well the deletion and deletion usually triggers some backend work as well. I know this because I did Promise.all as well and caused the whole platform outage in the past 😅

lojzatran · 2024-10-10T20:13:49Z

packages/platform-sdk/test/integration-tests/CleanEverythingIntegrationTest.ts

+          taxCategory.id,
+          taxCategory.version,
+          async () => {
+            const productsReferencingTaxCategory = await apiRoot


@evansinho I would recommend to delete straightaway and not checking.
Reasons:

At this point products are all delete. If not, then it should fail and you should investigate why. It could be an error in the script and then we have to fix it. If it's a platform error, then we should report it.

Checking this requires extra request/response. This script could be used as a reference for customers, so we should avoid doing unnecessary actions.

Please check also for other resources that use safeDeleteResource().

feat(cleanup): add bulk resource deletion script for commercetools (D…

ba6f8f2

…EVX-433) - cleanup all resources Refs DEVX-433

evansinho requested a review from a team as a code owner October 4, 2024 15:22

evansinho changed the title ~~feat(cleanup): add bulk resource deletion script for commercetools (DEVX-433)~~ feat(cleanup): add bulk resource deletion script for integration tests (DEVX-433) Oct 4, 2024

cneijenhuis reviewed Oct 7, 2024

View reviewed changes

lojzatran requested changes Oct 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cleanup): add bulk resource deletion script for integration tests (DEVX-433) #815

feat(cleanup): add bulk resource deletion script for integration tests (DEVX-433) #815

evansinho commented Oct 4, 2024

changeset-bot bot commented Oct 4, 2024

cneijenhuis Oct 7, 2024

evansinho Oct 7, 2024

cneijenhuis Oct 7, 2024

evansinho Oct 7, 2024

lojzatran Oct 10, 2024

ajimae commented Oct 10, 2024

codecov bot commented Oct 10, 2024

evansinho commented Oct 10, 2024

lojzatran left a comment

lojzatran Oct 10, 2024

lojzatran Oct 10, 2024

lojzatran Oct 10, 2024

lojzatran Oct 10, 2024

lojzatran Oct 10, 2024

lojzatran Oct 10, 2024

lojzatran Oct 10, 2024

feat(cleanup): add bulk resource deletion script for integration tests (DEVX-433) #815

Are you sure you want to change the base?

feat(cleanup): add bulk resource deletion script for integration tests (DEVX-433) #815

Conversation

evansinho commented Oct 4, 2024

changeset-bot bot commented Oct 4, 2024

⚠️ No Changeset found

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajimae commented Oct 10, 2024

codecov bot commented Oct 10, 2024

Codecov Report

evansinho commented Oct 10, 2024

lojzatran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment