Skip to content

Commit

Permalink
chore: Remove shelljs use from backup command (#37356)
Browse files Browse the repository at this point in the history
This is towards fixing `appsmithctl` into a modern component of the
project. First order of business is to get rid of the `shelljs`
dependency, since we don't really need it that seriously, and since it
does weird stuff with `require()` that we can't use `esbuild` to build
`appsmithctl`.

Further PRs will remove `shelljs` from other commands as well, and then
we'll remove `shelljs` from our dependencies.


## Automation

/test sanity

### 🔍 Cypress test results
<!-- This is an auto-generated comment: Cypress test results  -->
> [!TIP]
> 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/11814325698>
> Commit: 98261ee
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11814325698&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.Sanity`
> Spec:
> <hr>Wed, 13 Nov 2024 09:58:36 UTC
<!-- end of auto-generated comment: Cypress test results  -->


## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [x] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Improved backup process with enhanced error handling and asynchronous
checks for available disk space.
- New function for executing commands with output capture added to
utility functions.

- **Bug Fixes**
- Refined error messages for insufficient disk space and password
mismatches during backup.

- **Tests**
- Expanded test suite for backup utilities and command execution,
ensuring robust coverage of various scenarios.

- **Documentation**
	- Updated formatting of numeric constants for better readability.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
sharat87 authored Nov 14, 2024
1 parent d90faa2 commit 748a5ec
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 26 deletions.
27 changes: 14 additions & 13 deletions deploy/docker/fs/opt/appsmith/utils/bin/backup.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
const fsPromises = require('fs/promises');
const path = require('path');
const os = require('os');
const shell = require('shelljs');
const utils = require('./utils');
const Constants = require('./constants');
const logger = require('./logger');
Expand All @@ -16,17 +15,18 @@ async function run() {
let errorCode = 0;
let backupRootPath, archivePath, encryptionPassword;
let encryptArchive = false;

try {
const check_supervisord_status_cmd = '/usr/bin/supervisorctl >/dev/null 2>&1';
shell.exec(check_supervisord_status_cmd, function (code) {
if (code > 0) {
shell.echo('application is not running, starting supervisord');
shell.exec('/usr/bin/supervisord');
}
});
await utils.execCommandSilent(["/usr/bin/supervisorctl"]);
} catch (e) {
console.error('Supervisor is not running, exiting.');
process.exitCode = 1;
return;
}

try {
console.log('Available free space at /appsmith-stacks');
const availSpaceInBytes = getAvailableBackupSpaceInBytes();
const availSpaceInBytes = getAvailableBackupSpaceInBytes("/appsmith-stacks");
console.log('\n');

checkAvailableBackupSpace(availSpaceInBytes);
Expand Down Expand Up @@ -232,13 +232,14 @@ function getTimeStampInISO() {
return new Date().toISOString().replace(/:/g, '-')
}

function getAvailableBackupSpaceInBytes() {
return parseInt(shell.exec('df --output=avail -B 1 /appsmith-stacks | tail -n 1'), 10)
async function getAvailableBackupSpaceInBytes(path) {
const stat = await fsPromises.statfs(path);
return stat.bsize * stat.bfree;
}

function checkAvailableBackupSpace(availSpaceInBytes) {
if (availSpaceInBytes < Constants.MIN_REQUIRED_DISK_SPACE_IN_BYTES) {
throw new Error('Not enough space avaliable at /appsmith-stacks. Please ensure availability of atleast 2GB to backup successfully.');
throw new Error("Not enough space available at /appsmith-stacks. Please ensure availability of at least 2GB to backup successfully.");
}
}

Expand All @@ -259,4 +260,4 @@ module.exports = {
removeOldBackups,
getEncryptionPasswordFromUser,
encryptBackupArchive,
};
};
13 changes: 6 additions & 7 deletions deploy/docker/fs/opt/appsmith/utils/bin/backup.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ const Constants = require('./constants');
const os = require('os');
const fsPromises = require('fs/promises');
const utils = require('./utils');
const shell = require('shelljs');
const readlineSync = require('readline-sync');

describe('Backup Tests', () => {
Expand All @@ -13,19 +12,19 @@ test('Timestamp string in ISO format', () => {
expect(backup.getTimeStampInISO()).toMatch(/(\d{4})-(\d{2})-(\d{2})T(\d{2})\-(\d{2})\-(\d{2})\.(\d{3})Z/)
});

test('Available Space in /appsmith-stacks volume in Bytes', () => {
shell.exec = jest.fn((format) => '20');
const res = expect(backup.getAvailableBackupSpaceInBytes())
res.toBe(20)

test('Available Space in /appsmith-stacks volume in Bytes', async () => {
const res = expect(await backup.getAvailableBackupSpaceInBytes("/"))
res.toBeGreaterThan(1024 * 1024)
});

it('Checkx the constant is 2 GB', () => {
let size = 2 * 1024 * 1024 * 1024
expect(Constants.MIN_REQUIRED_DISK_SPACE_IN_BYTES).toBe(size)
});

it('Should throw Error when the available size is below MIN_REQUIRED_DISK_SPACE_IN_BYTES', () => {
let size = Constants.MIN_REQUIRED_DISK_SPACE_IN_BYTES - 1;
expect(() => {backup.checkAvailableBackupSpace(size)}).toThrow('Not enough space avaliable at /appsmith-stacks. Please ensure availability of atleast 2GB to backup successfully.');
expect(() => backup.checkAvailableBackupSpace(size)).toThrow();
});

it('Should not hould throw Error when the available size is >= MIN_REQUIRED_DISK_SPACE_IN_BYTES', () => {
Expand Down
6 changes: 3 additions & 3 deletions deploy/docker/fs/opt/appsmith/utils/bin/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ const LAST_ERROR_MAIL_TS = "/appsmith-stacks/data/backup/last-error-mail-ts"

const ENV_PATH = "/appsmith-stacks/configuration/docker.env"

const MIN_REQUIRED_DISK_SPACE_IN_BYTES = 2147483648 // 2GB
const MIN_REQUIRED_DISK_SPACE_IN_BYTES = 2_147_483_648 // 2GB

const DURATION_BETWEEN_BACKUP_ERROR_MAILS_IN_MILLI_SEC = 21600000 // 6 hrs
const DURATION_BETWEEN_BACKUP_ERROR_MAILS_IN_MILLI_SEC = 21_600_000 // 6 hrs

const APPSMITH_DEFAULT_BACKUP_ARCHIVE_LIMIT = 4 // 4 backup archives

Expand All @@ -27,4 +27,4 @@ module.exports = {
DURATION_BETWEEN_BACKUP_ERROR_MAILS_IN_MILLI_SEC,
APPSMITH_DEFAULT_BACKUP_ARCHIVE_LIMIT,
ENV_PATH
}
}
6 changes: 3 additions & 3 deletions deploy/docker/fs/opt/appsmith/utils/bin/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,10 @@ function execCommandSilent(cmd, options) {

const p = childProcess.spawn(cmd[0], cmd.slice(1), {
...options,
stdio: "ignore",
});

p.on("exit", (code) => {
p.on("close", (code) => {
if (isPromiseDone) {
return;
}
Expand All @@ -173,8 +174,7 @@ function execCommandSilent(cmd, options) {
return;
}
isPromiseDone = true;
console.error("Error running command", err);
reject();
reject(err);
});
});
}
Expand Down
22 changes: 22 additions & 0 deletions deploy/docker/fs/opt/appsmith/utils/bin/utils.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
const { describe, test, expect } = require("@jest/globals");
const utils = require("./utils");

describe("execCommandSilent", () => {

test("Runs a command", async () => {
await utils.execCommandSilent(["echo"]);
});

test("silences stdout and stderr", async () => {
const consoleSpy = jest.spyOn(console, "log");
await utils.execCommandSilent(["node", "--eval", "console.log('test')"]);
expect(consoleSpy).not.toHaveBeenCalled();
consoleSpy.mockRestore();
});

test("handles errors silently", async () => {
await expect(utils.execCommandSilent(["nonexistentcommand"]))
.rejects.toThrow();
});

});

0 comments on commit 748a5ec

Please sign in to comment.