chore: merged implemented feedback (#3)

Implemented feedback

Author: AuHau

.travis.yml

@@ -36,6 +36,13 @@ jobs:
         firefox: latest
       script: npx aegir test -t browser -- --browsers FirefoxHeadless
 
+    - stage: test
+      name: sharness
+      os:
+        - linux
+        - osx
+      script: cd ./test/sharness && make
+
 notifications:
   email: false
 

README.md

@@ -77,30 +77,30 @@ Example:
 
 ```js
 const migrations = require('ipfs-repo-migrations')
-const getVersion = require('ipfs-repo-migrations/repo/version')
 
 const repoPath = 'some/repo/path'
-const repoVersion = await getVersion(repoPath)
+const currentRepoVersion = 7
+const latestVersion = migrations.getLatestMigrationVersion()
 
-if(repoVersion < migrations.getLatestMigrationVersion()){
+if(currentRepoVersion < latestVersion){
   // Old repo! Lets migrate to latest version!
-  await migrations.migrate(repoPath)
+  await migrations.migrate(repoPath, latestVersion)
 }
 ```
 
 To migrate your repository using the CLI, see the [how to run migrations](./run.md) tutorial. 
 
 ## API
 
-### `.migrate(path, {toVersion, ignoreLock, repoOptions, onProgress, isDryRun}) -> Promise<void>`
+### `.migrate(path, toVersion, {ignoreLock, repoOptions, onProgress, isDryRun}) -> Promise<void>`
 
 Executes a forward migration to a specific version, or to the latest version if a specific version is not specified.
 
 **Arguments:**
 
  * `path` (string, mandatory) - path to the repo to be migrated
+ * `toVersion` (int, mandatory) - version to which the repo should be migrated.
  * `options` (object, optional) - options for the migration
- * `options.toVersion` (int, optional) - version to which the repo should be migrated. Defaults to the latest migration version.
  * `options.ignoreLock` (bool, optional) - if true will not lock the repo when applying migrations. Use with caution.
  * `options.repoOptions` (object, optional) - options that are passed to migrations, that use them to construct the datastore. (options are the same as for IPFSRepo).
  * `options.onProgress` (function, optional) - callback that is called after finishing execution of each migration to report progress.
@@ -115,7 +115,7 @@ Signature of the progress callback.
  * `counter` (int) - index of current migration.
  * `totalMigrations` (int) - total count of migrations that will be run.
 
-### `.revert(path, toVersion, {ignoreLock, options, onProgress, isDryRun}) -> Promise<void>`
+### `.revert(path, toVersion, {ignoreLock, repoOptions, onProgress, isDryRun}) -> Promise<void>`
 
 Executes backward migration to a specific version.
 
@@ -125,7 +125,7 @@ Executes backward migration to a specific version.
  * `toVersion` (int, mandatory) - version to which the repo should be reverted to. 
  * `options` (object, optional) - options for the reversion
  * `options.ignoreLock` (bool, optional) - if true will not lock the repo when applying migrations. Use with caution.
- * `options.options` (object, optional) - options that are passed to migrations, that use them to construct the datastore. (options are the same as for IPFSRepo).
+ * `options.repoOptions` (object, optional) - options that are passed to migrations, that use them to construct the datastore. (options are the same as for IPFSRepo).
  * `options.onProgress` (function, optional) - callback that is called after finishing execution of each migration to report progress.
  * `options.isDryRun` (bool, optional) - flag that indicates if it is a dry run that should give the same output as running a migration but without making any actual changes.
 
@@ -168,33 +168,40 @@ every run of `jsipfs-migrations add` (manual changes should follow the same styl
 
 Each migration must follow this API. It must export an object in its `index.js` that has following properties:
 
- * `version` (int) - Number that represents the version which the repo will migrate to (eg. `migration-8` will move the repo to version 8).
+ * `version` (int) - Number that represents the version which the repo will migrate to (eg. `8` will move the repo to version 8).
  * `description` (string) - Brief description of what the migrations does.
  * `migrate` (function) - Function that performs the migration (see signature of this function below)
  * `revert` (function) - If defined then this function will revert the migration to the previous version. Otherwise it is assumed that it is not possible to revert this migration.
 
-#### `.migrate(repoPath, isBrowser)`
+#### `.migrate(repoPath, repoOptions)`
 
 _Do not confuse this function with the `require('ipfs-repo-migrations').migrate()` function that drives the whole migration process!_
 
 Arguments:
  * `repoPath` (string) - absolute path to the root of the repo
- * `options` (object, optional) - object containing `IPFSRepo` options, that should be used to construct a datastore instance.
- * `isBrowser` (bool) - indicates if the migration is run in a browser environment (as opposed to NodeJS)
+ * `repoOptions` (object, optional) - object containing `IPFSRepo` options, that should be used to construct a datastore instance.
 
-#### `.revert(repoPath, isBrowser)`
+#### `.revert(repoPath, repoOptions)`
 
 _Do not confuse this function with the `require('ipfs-repo-migrations').revert()` function that drives the whole backward migration process!_
 
 Arguments:
  * `repoPath` (string) - path to the root of the repo
- * `options` (object, optional) - object containing `IPFSRepo` options, that should be used to construct the datastore instance.
- * `isBrowser` (bool) - indicates if the migration is run in a browser environment (as opposed to NodeJS)
+ * `repoOptions` (object, optional) - object containing `IPFSRepo` options, that should be used to construct the datastore instance.
 
 ### Browser vs. NodeJS environments
 
-The migration might need to distinguish in which environment it runs (browser vs. NodeJS). For this reason there is an argument
-`isBrowser` passed to migrations functions. But with simple migrations it should not be necessary to distinguish between
+The migration might need to perform specific tasks in browser or NodeJS environment. In such a case create
+migration file `/migrations/migration-<number>/index_browser.js` which have to follow the same API is described before.
+Then add entry in `package.json` to the `browser` field as follow:
+
+```
+'./migrations/migration-<number>/index.js': './migrations/migration-<number>/index_browser.js'
+```
+
+In browser environments then `index.js` will be replaced with `index_browser.js`.
+
+Simple migrations should not need to distinguish between
 these environments as the datastore implementation will handle the main differences. 
 
 There are currently two main datastore implementations:
@@ -221,8 +228,8 @@ write the rest of the migration!
 
 ### Integration with js-ipfs
 
-When a new migration is created, the repo version in [`js-ipfs-repo`](https://github.com/ipfs/js-ipfs-repo) should be updated with the new version,
-together with updated version of this package. Then the updated version should be propagated to `js-ipfs`.
+When a new migration is created, new version of this package have to be released. Afterwards version of this package in [`js-ipfs-repo`](https://github.com/ipfs/js-ipfs-repo) have to be updated
+together with the repo version that `IPFSRepo` expects. Then the updated version of `js-ipfs-repo` should be propagated to `js-ipfs`.
 
 ### Tests
 
@@ -259,7 +266,9 @@ This will create an empty migration with the next version.
 
 ### Module versioning notes 
 
-In order to have good overview of what version of package contains what kind of migrations, this package follows this versioning schema: `0.<versionOfLastMigration>.<patches>`.
+In order to have good overview of what version of package contains what kind of migration, to every release there
+should be appended version's metadata in format `migr-<versionOfLatestMigration>`. If for releasing is used `aegir`
+you can use the `release --metadata` option.
 
 ## Contribute
 

maintainer.json

@@ -0,0 +1,11 @@
+{
+  "repoLeadMaintainer": {
+    "name": "Adam Uhlíř",
+    "email": "[email protected]",
+    "username": "auhau"
+  },
+  "workingGroup": {
+    "name": "JS IPFS",
+    "entryPoint": "https://github.com/ipfs/js-core"
+  }
+}

package.json

@@ -12,15 +12,13 @@
     "url": "https://github.com/ipfs/js-ipfs-repo-migrations/issues/"
   },
   "license": "MIT",
-  "leadMaintainer": "Adam Uhlir <[email protected]>",
   "files": [
     "migrations",
     "src",
     "dist"
   ],
   "main": "src/index.js",
   "browser": {
-    "./src/option-node.js": "./src/option-browser.js",
     "./src/repo/lock.js": "./src/repo/lock-memory.js",
     "datastore-fs": "datastore-level"
   },

run.md

@@ -13,7 +13,7 @@ If you run into any trouble, please feel free to [open an issue in this reposito
 The migration tool is safe -- it should not delete any data. If you have important data stored _only_ in your ipfs node, and want to be extra safe, you can back up the whole repo with:
 
 ```sh
-cp -r ~/.js-ipfs ~/.js-ipfs.bak
+cp -r ~/.jsipfs ~/.jsipfs.backup
 ```
 
 ## Step 1. Downloading the Migration
@@ -26,16 +26,16 @@ To run the migration tool:
 
 ```sh
 # to check if there are migrations that need to be applied
-js-ipfs-repo-migrations status
+jsipfs-repo-migrations status
 
 # if so, migrate to the latest version
-js-ipfs-repo-migrations migrate
+jsipfs-repo-migrations migrate
 ```
 
 ## Step 3. Done! Run IPFS.
 
 If the migration completed without error, then you're done! Try running the new ipfs:
 
 ```
-js-ipfs daemon
+jsipfs daemon
 ```

src/commands.js

@@ -5,17 +5,33 @@ const path = require('path')
 const fs = require('fs')
 const process = require('process')
 const util = require('util')
+const readline = require('readline')
 
 const writeFile = util.promisify(fs.writeFile)
 const mkdir = util.promisify(fs.mkdir)
 
 const chalk = require('chalk')
 
 const repoVersion = require('./repo/version')
+const migratorUtils = require('./utils')
 const migrator = require('./index')
 const templates = require('./migration-templates')
 const migrations = require('../migrations')
 
+function shouldContinueWithIrreversibleMigration () {
+  return new Promise((resolve, reject) => {
+    const rl = readline.createInterface({
+      input: process.stdin,
+      output: process.stdout
+    })
+
+    rl.question(chalk.yellow('There is irreversible migration in the migrations plan. Do you want to continue? [y/N]'), (answer) => {
+      rl.close()
+      resolve(answer === 'y')
+    })
+  })
+}
+
 function asyncClosure (fnc) {
   return function asyncWrapper ({ resolve, ...options }) {
     resolve(fnc(options))
@@ -29,14 +45,10 @@ function reportingClosure (action) {
 
 async function migrate ({ repoPath, migrations, to, dry, revertOk }) {
   repoPath = repoPath || process.env.IPFS_PATH || path.join(os.homedir(), '.jsipfs')
-
-  // Import migrations if set
-  if (migrations) {
-    migrations = require(migrations)
-  }
+  migrations = migrations === undefined ? require('../migrations') : require(migrations)
 
   if (!to) {
-    to = migrator.getLatestMigrationVersion()
+    to = migrator.getLatestMigrationVersion(migrations)
   }
 
   const version = await repoVersion.getVersion(repoPath)
@@ -63,7 +75,13 @@ async function migrate ({ repoPath, migrations, to, dry, revertOk }) {
   if (version === to) {
     return chalk.yellow('Nothing to migrate! Versions matches')
   } else if (version < to) {
-    await migrator.migrate(repoPath, options)
+    if (migratorUtils.containsIrreversibleMigration(version, to, migrations)) {
+      if (!(await shouldContinueWithIrreversibleMigration())) {
+        return 'Migration aborted'
+      }
+    }
+
+    await migrator.migrate(repoPath, to, options)
   } else if (revertOk) {
     await migrator.revert(repoPath, to, options)
   } else {

src/errors.js

@@ -16,21 +16,6 @@ class NonReversibleMigrationError extends Error {
 NonReversibleMigrationError.code = 'ERR_NON_REVERSIBLE_MIGRATION'
 exports.NonReversibleMigrationError = NonReversibleMigrationError
 
-/**
- * Exception raised when structure of a repo is not as expected.
- */
-class UnknownRepoStructureError extends Error {
-  constructor (message) {
-    super(message)
-    this.name = 'UnknownRepoStructureError'
-    this.code = 'ERR_UNKNOWN_REPO_STRUCTURE'
-    this.message = message
-  }
-}
-
-UnknownRepoStructureError.code = 'ERR_UNKNOWN_REPO_STRUCTURE'
-exports.UnknownRepoStructureError = UnknownRepoStructureError
-
 /**
  * Exception raised when repo is not initialized.
  */