feat:(CLI) add delete app, bundle and entry points by thabti · Pull Request #75 · staticdeploy/staticdeploy

thabti · 2021-09-28T08:51:12Z

issue: #74

Adding CLI to support delete app, bundle and all attached entry points

usage:

export STATICDEPLOY_API_URL=http://abc.com/api
./bin/staticdeploy.js delete --app abc-master --tag master

pscanf · 2021-09-29T06:59:31Z

Hello @sabeurthabti , great PR, thanks!

Sorry had a busy day yesterday and didn't get around to review it yet, but I should be able to take a look at it tonight.

For updating the website and the documentation, I think the only two files that need updating are the reference documentation for the CLI, and the CLI's README.

thabti · 2021-09-29T10:40:49Z

No rush mate, thanks for the support. I'd like to contribute more, getting understanding of the architecture.
I couldn't understand why the build was failing, will spend some time to fix the build also.

thabti · 2021-10-03T08:19:39Z

@pscanf any update on this? the ci command runs perfectly fine on my machine. I tried to spend time to debug it .. not luck.

pscanf · 2021-10-03T08:47:45Z

Hadn't looked at the build yet, sorry. Definitely an odd error.

Seeing that it's thrown by the aws-sdk lib, and considering its version is pinned (so there cannot have been rogue updates) I would point my finger at the system aws-sdk talks to, i.e. the minio/minio container. From the docker hub image page, it seems they published a few new versions recently, you could try pulling the latest tag on your dev machine and see if you can replicate the error. If that does it, we can either try to update the aws-sdk lib, or downgrade the minio server.

Unfortunately these last days I got a bit swamped with work, so I can't offer much help until the second half of next week.

pscanf · 2021-10-04T19:37:50Z

As I suspected, the issue is caused by newer versions of minio, that behave differently when an "empty" deleteObjects operation is issued:

s3Client.deleteObjects({
  Bucket: "bucket",
  Delete: { Objects: [] },
})

Maybe this was not a legal operation to begin with, but older versions of minio apparently didn't mind and returned a positive response, while newer versions do not (I did not try to figure out when the change happened).

In any case, I updated the integration test so that it won't issue such operations, and also updated the aws-sdk dependency for good measure.

Some CI steps still fail, but I think it's because - for security reasons - some secret env variables are not injected in the PR pipelines, so those steps should succeed when the PR is merged.

Alas, still haven't reviewed the code, will try to do that tomorrow.

pscanf

Other than a few nitpicks, I have one change request and one doubt.

Change request

Split the delete command in two commands deleteAppWithEntrypoints (or maybe cleanupApp, or teardownApp) and deleteBundles.

Motivation

There is no connection between apps and bundles. They might have the same name, but it's by choice of whoever creates them, not a general rule. I could, for example, have bundles for my webapp my-webapp, which I sell to two different clients and deploy to the two apps client-A and client-B.

Doubt

The deleteApp command deletes entrypoints one by one, then deletes the app. This process could fail at any point, leaving StaticDeploy in an "unexpected" state. That is, the developer expects the app and all its entrypoints to have been deleted, but only a few entrypoints were deleted, while the others and the app still exist.

Solution

The ideal solution would be to move the process to the server, adding a CleanupAppByName usecase which issues all the delete commands in a transaction. However, this would greatly complicate things, so I think that given the unlikelihood of these failures, and given the relative harmless consequences, we can stick to the current implementation. I would however add some logs, for instance after each entrypoint deletion, so that it's easier for the user to see at which point of the process things went wrong (there are operation logs for that as well, but more feedback doesn't hurt).

pscanf · 2021-10-10T10:27:15Z

cli/src/commands/delete.ts

+                // Read the config file
+                const config = readStaticdeployConfig(configPath);
+
+                // Return the deploy config, defaulting to an empty object


Nit: ~~deploy~~delete

pscanf · 2021-10-10T10:32:33Z

website/docs/reference-cli.md

    // Options for the deploy command as specified above
  },
+  delete: {
+    // Options for the deploy command as specified above


Nit: ~~deploy~~delete

pscanf · 2021-10-10T10:35:24Z

cli/src/commands/delete.ts

+        });
+
+        const apps = await client.apps.getAll();
+        const app = apps.find((item) => item.name === argv.app);


Here you should handle the possibility of the app not existing, occurring for instance when the user specifies a non-existing app name, and throwing an error.

pscanf · 2021-10-10T10:38:49Z

cli/src/commands/delete.ts

+        const apps = await client.apps.getAll();
+        const app = apps.find((item) => item.name === argv.app);
+        const appId = String(app?.id);
+        const appEntries = await client.entrypoints.getAll({ appId });


~~appEntries~~ entrypoints

pscanf · 2021-10-10T10:40:47Z

cli/src/commands/delete.ts

+        const appId = String(app?.id);
+        const appEntries = await client.entrypoints.getAll({ appId });
+
+        for (const index of appEntries.keys()) {


for (const { id } of entrypoints) { await client.entrypoints.delete(id); }

pscanf · 2021-10-10T10:46:46Z

cli/test/commands/delete.ts

@@ -0,0 +1,168 @@
+// import StaticdeployClient from "@staticdeploy/sdk";


Nit: remove commented import

pscanf · 2021-10-10T11:32:36Z

(Btw, sorry for the delay in reviewing, busy work period)

thabti · 2021-10-11T07:22:10Z

thanks fo rthe comments, will spend today resolving them. @pscanf

feat: add delete app, bundle and entry points

fbd7833

thabti marked this pull request as draft September 28, 2021 08:51

thabti marked this pull request as ready for review September 28, 2021 08:51

thabti changed the title ~~feat: add delete app, bundle and entry points~~ feat:(CLI) add delete app, bundle and entry points Sep 28, 2021

thabti added 3 commits September 28, 2021 13:09

fix lint and code improvements

058a520

chore:make the code simple

48f1948

increase test coverage

2e52ffc

thabti added 2 commits September 29, 2021 14:55

fix: update read me and fix description

9cc4b14

fix: update read me and fix description

ad90143

fix: workaround minio edge-case causing S3 integration test failure

dd5ef31

pscanf requested changes Oct 10, 2021

View reviewed changes

		@@ -0,0 +1,168 @@
		// import StaticdeployClient from "@staticdeploy/sdk";

Conversation

thabti commented Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pscanf commented Sep 29, 2021

Uh oh!

thabti commented Sep 29, 2021

Uh oh!

thabti commented Oct 3, 2021

Uh oh!

pscanf commented Oct 3, 2021

Uh oh!

pscanf commented Oct 4, 2021

Uh oh!

pscanf left a comment

Choose a reason for hiding this comment

Change request

Motivation

Doubt

Solution

Uh oh!

pscanf Oct 10, 2021

Choose a reason for hiding this comment

Uh oh!

pscanf Oct 10, 2021

Choose a reason for hiding this comment

Uh oh!

pscanf Oct 10, 2021

Choose a reason for hiding this comment

Uh oh!

pscanf Oct 10, 2021

Choose a reason for hiding this comment

Uh oh!

pscanf Oct 10, 2021

Choose a reason for hiding this comment

Uh oh!

pscanf Oct 10, 2021

Choose a reason for hiding this comment

Uh oh!

pscanf commented Oct 10, 2021

Uh oh!

thabti commented Oct 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thabti commented Sep 28, 2021 •

edited

Loading