[HOLD]✨Backup subcommand #474

vikaspotluri123 · 2017-09-12T22:45:49Z

Adds the ability to run ghost backup to create a zip archive of your important data - the config file, your content folder, and your database

ref: #468

Todo List

Create functions for each core task
Add UI logging
Write test suite
Integrate w/ extensions

coveralls · 2017-09-12T22:54:46Z

Coverage remained the same at 72.291% when pulling 68470a9 on vikaspotluri123:backup into b5f9c44 on TryGhost:master.

I thought I did this, but apparently not 😕

coveralls · 2017-09-12T23:05:32Z

Coverage remained the same at 72.291% when pulling f65fbfe on vikaspotluri123:backup into b5f9c44 on TryGhost:master.

acburdine

Definitely a good start! Just needs some fixes and tests.

The only other thing I can think of is it would probably be good to copy other things out too - e.g. the nginx and systemd config files. However, that's probably better added as an extension hook, so that the systemd and nginx extensions can do it themselves. (You don't need to implement this hook if you don't want - I can take care of it later 😁)

lib/commands/backup.js

+        let exporter;
+        let saveLocation = process.cwd();
+
+        // @todo check if we need to ensure we're in a ghost directory


lib/commands/backup.js

+
+        // @todo check if we need to ensure we're in a ghost directory
+
+        if (argv.output) { // @todo add o as an alias


lib/commands/backup.js

+
+        instance.checkEnvironment();
+
+        if (instance.running()) {


lib/commands/backup.js

+            this.ui.log('Ghost is currently running. Backing up might take longer and slow down your site','yellow');
+        }
+
+        const backupData = {


lib/commands/backup.js

+
+        const props = {
+            database: exporter.doExport({}),
+            environment: this.system.environment


lib/commands/backup.js

+            environment: this.system.environment
+        };
+
+        return Promise.props(props).then((data) => {


lib/commands/backup.js

+                zipFile.writeZip(saveLocation);
+                debug('Wrote zipFile')
+
+                return Promise.resolve(saveLocation);


- No longer using the `.ghost-backup` file; that's redundant - Use listr to log tasks - Separate tasks into their own function - Add support for backing up plugins (will be properly implemented in the future) - Add error check in backup - Workaround adm-zip bug

Needs a LOT of modifications

coveralls · 2017-09-16T18:52:46Z

Coverage remained the same at 77.432% when pulling 5ee8edf on vikaspotluri123:backup into 3ac283c on TryGhost:master.

acburdine

This is definitely looking a lot better! Really just needs the directory traversing for the content folder and then tests 😄

lib/commands/backup.js

+    backupContent() {
+        // Add content folder // @todo: Do we need to check if the content folder exists?
+        // @todo: Figure out how to deal with Windows bug
+        this.zipFile.addLocalFolder(path.join(process.cwd(), 'content/'), 'content/');


- Use fs-extra for ensureDirSync - Meet linting requirements - saveLocation needs to append name after all mods -- Add instance name to save - Add and use klaw-sync to walk content folder (Windows zip bug) - Remove stray debugs - Plugins are really extensions

I have a feeling the efficiency can be improved, but for now, this is what I got!

coveralls · 2017-09-27T05:52:35Z

Coverage remained the same at 77.432% when pulling 6200a6e on vikaspotluri123:backup into 3ac283c on TryGhost:master.

coveralls · 2017-09-27T05:53:58Z

Coverage increased (+0.8%) to 78.229% when pulling 6200a6e on vikaspotluri123:backup into 3ac283c on TryGhost:master.

Just sorted tests for easier legibility Moved some constants to global scope

coveralls · 2017-09-28T02:26:34Z

Coverage increased (+0.8%) to 78.229% when pulling ce92736 on vikaspotluri123:backup into 3ac283c on TryGhost:master.

coveralls · 2017-09-28T02:49:46Z

Coverage increased (+0.8%) to 78.229% when pulling 8865334 on vikaspotluri123:backup into 3ac283c on TryGhost:master.

acburdine

Definitely looking really good!

Just a couple of stylistic notes in the actual code; however I've got a couple of other notes that might be able to help clean up the tests.

At the moment, you're writing things in each of the tasks to this, which works and is not necessarily bad practice, but it makes it very difficult to test each task individually, which is the point of unit tests 😄

My recommendation would be in all of the functions you're currently writing something to this, e.g.this.zipFile, this.saveLocation, etc. - you instead pass a context object via listr, and write stuff to that instead. Doing so would allow you to run the individual tasks in your unit tests, instead of running all of the listr tasks at once, even though you're only testing the one task.

lib/commands/backup.js

+
+class BackupCommand extends Command {
+    run(argv) {
+        return this.ui.listr([{task: this.initialize.bind(this),


lib/commands/backup.js

+            files = walker('./content', {nodir: true});
+        } catch (E) {
+            debug(`Failed reading content folder: ${E}`);
+            return Promise.reject(new Error('Failed to read content folder'));


package.json

@@ -53,6 +53,7 @@
    "ghost-ignition": "2.8.14",
    "inquirer": "3.2.3",
    "is-running": "2.1.0",
+    "klaw-sync": "^3.0.0",


- Use ctx to store backup data - For unit tests, test a single function, rather than the command each time - Force version for klaw

coveralls · 2017-09-30T18:35:02Z

Coverage increased (+0.8%) to 78.229% when pulling 01458c1 on vikaspotluri123:backup into 3ac283c on TryGhost:master.

coveralls · 2017-10-09T00:03:25Z

Coverage increased (+0.9%) to 78.345% when pulling b75070d on vikaspotluri123:backup into 3ac283c on TryGhost:master.

acburdine · 2017-10-09T16:14:25Z

This looks really good 😄 I'll pull this down and test later tonight!

braunsonm · 2017-10-25T04:51:37Z

Awesome! @vikaspotluri123

coveralls · 2017-11-07T03:13:39Z

Coverage increased (+0.7%) to 82.277% when pulling f81d7e5 on vikaspotluri123:backup into 53a3425 on TryGhost:master.

acburdine · 2017-11-18T17:52:42Z

@vikaspotluri123 can you go ahead and rebase this? pulling it down and testing today but it says there are merge conflicts :/

acburdine

Sorry it's taken so long to get to this!

Just pulled it down to test and ran into a few issues with it - which I've commented on :)

lib/commands/backup.js

+            env: this.system.environment,
+            argv: argv,
+            instance: instance,
+            hook: this.system.hook


lib/commands/backup.js

+            title: 'Writing backup file',
+            task: this.writeZipFile
+        }],{
+            env: this.system.environment,


lib/commands/backup.js

+            }
+        });
+
+        return Promise.resolve();


lib/commands/backup.js

+            const location = file.path.replace(/\\/g,'/');
+            let dir = path.dirname(location);
+            const name = path.basename(location);
+            dir = path.relative(cwd, dir);


coveralls · 2017-11-18T22:04:06Z

Coverage increased (+0.5%) to 89.282% when pulling 3e7235b on vikaspotluri123:backup into 8a96fa1 on TryGhost:master.

… skip]

coveralls · 2017-11-19T05:06:52Z

Coverage increased (+0.6%) to 89.381% when pulling 534fc53 on vikaspotluri123:backup into 8a96fa1 on TryGhost:master.

vikaspotluri123 · 2017-11-19T05:07:33Z

👍 Done @acburdine

lib/commands/backup.js

+        const read = fs.readFileSync;
+
+        return ctx.system.hook('backup').then((extensionFiles) => {
+            each(extensionFiles, (filesToAdd) => {


coveralls · 2017-11-20T14:17:31Z

Coverage increased (+0.6%) to 89.387% when pulling 4ae45ff on vikaspotluri123:backup into 8a96fa1 on TryGhost:master.

test/utils/env.js

@@ -52,18 +52,18 @@ module.exports = function setupEnv(typeOrDefinition, dir) {
        });
    }

-    if (setup.links) {


coveralls · 2017-11-28T22:29:09Z

Coverage increased (+0.6%) to 89.41% when pulling 161003e on vikaspotluri123:backup into 5ed1cfe on TryGhost:master.

lib/commands/backup.js

+
+        if (ctx.instance.running()) {
+            ctx.instance.ui.log('Ghost is currently running. Backing up might take longer and slow down your site', 'yellow');
+            ctx.instance.loadRunningEnvironment();


lib/commands/backup.js

+
+    backupDatabase(ctx) {
+        debug('Creating database backup');
+        let exporter;


lib/commands/backup.js

+        folder = folder || './content';
+
+        // path MUST be relative
+        if (folder.indexOf('./') !== 0) {


xmrcheck · 2019-03-04T22:01:14Z

Hello, can we know why this feature is on hodl?
Thanks.

acburdine · 2019-03-04T23:20:38Z

@xmrcheck apologies for the delay in getting this feature out. Given that this command is non-trivial, we will need to draft a formal spec for the command first before getting this merged in. Will try to get that done sometime soon.

github-actions · 2021-05-03T15:03:13Z

Our bot has automatically marked this PR as stale because there has not been any activity here in some time. If we’ve failed to review your PR & you’re still interested in working on it, please let us know. Otherwise this PR will be closed shortly, but can always be reopened later. Thank you for understanding 🙂

vikaspotluri123 added 2 commits September 9, 2017 14:00

Merge remote-tracking branch 'refs/remotes/TryGhost/master'

3293f3f

Add backup command logic

68470a9

Fix lint errors

f65fbfe

I thought I did this, but apparently not 😕

acburdine requested changes Sep 14, 2017

View reviewed changes

vikaspotluri123 added 3 commits September 14, 2017 18:15

Merge remote-tracking branch 'refs/remotes/TryGhost/master' into backup

2f4e7a5

Fix bugs, cleanup

c680215

- No longer using the `.ghost-backup` file; that's redundant - Use listr to log tasks - Separate tasks into their own function - Add support for backing up plugins (will be properly implemented in the future) - Add error check in backup - Workaround adm-zip bug

Remove backup-spec

5ee8edf

Needs a LOT of modifications

acburdine reviewed Sep 20, 2017

View reviewed changes

vikaspotluri123 added 2 commits September 27, 2017 00:49

🎉 Add Tests

6200a6e

I have a feeling the efficiency can be improved, but for now, this is what I got!

Itemize tests

ce92736

Just sorted tests for easier legibility Moved some constants to global scope

Fix issues with UI stub (global)

8865334

acburdine requested changes Sep 29, 2017

View reviewed changes

Clean up tests & backup command

01458c1

- Use ctx to store backup data - For unit tests, test a single function, rather than the command each time - Force version for klaw

Add support for extensions

b75070d

Merge branch 'master' into backup

f81d7e5

vikaspotluri123 changed the title ~~[WIP] ✨Backup subcommand~~ ✨Backup subcommand Nov 10, 2017

Merge branch 'master' into backup

4610f99

acburdine self-assigned this Nov 18, 2017

acburdine requested changes Nov 18, 2017

View reviewed changes

Merge remote-tracking branch 'TryGhost/master' into backup

3e7235b

vikaspotluri123 added 6 commits November 18, 2017 16:06

Pass system object rather than specific properties [ci skip]

2be5932

Add symlink support for content, don't overwrite existing backups [ci…

10f6366

… skip]

[ci skip] env (TryGhost#549) - HOLD

c798d19

Give more descriptive naming of relative file

5b8cbd3

Finish up tests and minor cleanup

d2d9d4b

Forgot to lint

534fc53

acburdine reviewed Nov 20, 2017

View reviewed changes

lib/commands/backup.js Outdated

const read = fs.readFileSync;

return ctx.system.hook('backup').then((extensionFiles) => {

each(extensionFiles, (filesToAdd) => {

This comment was marked as abuse.

Sign in to view

chore(reqs): remove unnecessary call to lodash/each

4ae45ff

vikaspotluri123 commented Nov 23, 2017

View reviewed changes

test/utils/env.js Outdated

@@ -52,18 +52,18 @@ module.exports = function setupEnv(typeOrDefinition, dir) {

});

}

if (setup.links) {

This comment was marked as abuse.

Sign in to view

Merge remote-tracking branch 'tryghost/master' into backup

161003e

acburdine reviewed Dec 1, 2017

View reviewed changes

lib/commands/backup.js Outdated

if (ctx.instance.running()) {

ctx.instance.ui.log('Ghost is currently running. Backing up might take longer and slow down your site', 'yellow');

ctx.instance.loadRunningEnvironment();

This comment was marked as abuse.

Sign in to view

acburdine reviewed Dec 1, 2017

View reviewed changes

lib/commands/backup.js

backupDatabase(ctx) {

debug('Creating database backup');

let exporter;

This comment was marked as abuse.

Sign in to view

Update environment globally

66a78fa

vikaspotluri123 changed the title ~~✨Backup subcommand~~ [HOLD]✨Backup subcommand Dec 3, 2017

kirrg001 reviewed Dec 4, 2017

View reviewed changes

lib/commands/backup.js

folder = folder || './content';

// path MUST be relative

if (folder.indexOf('./') !== 0) {

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

m1guelpf mentioned this pull request Oct 12, 2018

[Feat] Created "backup" command to create backups for db, files or both #842

Closed

github-actions bot added the Stale label May 3, 2021

vikaspotluri123 closed this May 3, 2021


		// @todo check if we need to ensure we're in a ghost directory

		if (argv.output) { // @todo add o as an alias

[HOLD]✨Backup subcommand #474

[HOLD]✨Backup subcommand #474

Conversation

vikaspotluri123 commented Sep 12, 2017 • edited Loading

Todo List

coveralls commented Sep 12, 2017

coveralls commented Sep 12, 2017

acburdine left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

coveralls commented Sep 16, 2017

acburdine left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

coveralls commented Sep 27, 2017

coveralls commented Sep 27, 2017

coveralls commented Sep 28, 2017

coveralls commented Sep 28, 2017

acburdine left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

coveralls commented Sep 30, 2017

coveralls commented Oct 9, 2017

acburdine commented Oct 9, 2017

braunsonm commented Oct 25, 2017

coveralls commented Nov 7, 2017

acburdine commented Nov 18, 2017

acburdine left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

coveralls commented Nov 18, 2017

coveralls commented Nov 19, 2017

vikaspotluri123 commented Nov 19, 2017

This comment was marked as abuse.

coveralls commented Nov 20, 2017

This comment was marked as abuse.

coveralls commented Nov 28, 2017

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

xmrcheck commented Mar 4, 2019

acburdine commented Mar 4, 2019

github-actions bot commented May 3, 2021

vikaspotluri123 commented Sep 12, 2017 •

edited

Loading