Specifying migration name should not fail if migration has already been run #167
Comments
|
I'd be happy to submit a PR along these lines if this is something you'd be open to having changed. |
|
I am suffering from the same error and totally agree with your thoughts on this @chadxz Is anyone looking into how to fix this at all? |
|
Yes we're discussing this. Existing flows might be depending on the current behaviour, making this a breaking change. So any update will be in v3, which isn't finalised yet. We can use this issue to discuss what the behaviour should be. As I see it there are three options, when an already-run migration is explicitly requested:
It sounds like the request here is 2. Is that right? |
|
@mmkal Yes, option 2 would be preferable. You could add an option to control this behaviour to avoid breaking any existing code that relied on the old behaviour - e.g.: where the default value would be |
|
FWIW, my use case was this: I needed to run a single migration, then run some custom logic, then run the remainder of the migrations. I was able to work around the issue in this ticket with this: I created a npm run script with the following: Then checked the output of that command to determine whether it should run or not.
To me that would be the least surprising thing, which is always good 👍 |
|
Hi @asnaseer-resilient and @chadxz, I think it is a good idea to have this configurable, and the default choice could be the one such that no breaking change happens. @mmkal what do you think? |
mmkal
added a commit
that referenced
this issue
Oct 7, 2020
Closes #233 (replacement PR) Closes #106 - multiple folders now supported via globbing Closes #169 - shouldn't be as necessary anymore, but potentially further work could be done to make it more convenient to handle many different kinds of migrations Closes #188 - migration class removed Closes #193 - since we're now using `glob`, we can find migrations in symlinked directories. If necessary, in a follow-up we can expose the symlink-related glob options, but there are gotchas there so let's wait for a user request Fixes #302 - `pattern` option is removed in favour of explicit globs Fixes #259 - user is now responsible for globbing/ignoring migration files Closes #185 - although closes as "wontfix" - array support is still gone. `Promise.all` should be used Fixes #171 Touches on #167 - but should continue discussion there as this still doesn't introduce a config option for silently skipping already-applied migrations that are explicitly specified Fixes #33 This is more-or-less a rewrite of the `Umzug` class, consolidating and simplifying several options. Minimal usage now: ```js import { Umzug } from 'umzug' const umzug = new Umzug({ migrations: { glob: 'path/to/migrations/*.js' }, logger: console, }) ``` Note: the `umzug.ts` file is collapsed in GitHub's diff, but that's where the main change is, so it should be opened! TODO: - [x] audit existing issues - many can likely be closed by this - [ ] decide on whether we should make sure consumer is returning a promise. increases test overhead but stops users shooting themselves in the foot. see #233 (comment) - [ ] decide if we want to allow no-oping already-run migrations per #167 - [x] narrow logger interface to only take string messages - can be widened in future, but not narrowed - [ ] document how to support multiple folders. this is a common feature request and globbing supports it via `'{path1/*.js,path2/*.js}'` Summary of the changes, from the readme in the changeset: ___ The Umzug class should be imported as a named import, i.e. `import { Umzug } from 'umzug'`. The `MigrationMeta` type, which is returned by `umzug.executed()` and `umzug.pending()`, no longer has a `file` property - it has a `name` and *optional* `path` - since migrations are not necessarily bound to files on the file system. The `migrations.glob` parameter replaces `path`, `pattern` and `traverseDirectories`. It can be used, in combination with `cwd` and `ignore` to do much more flexible file lookups. See https://npmjs.com/package/glob for more information on the syntax. The `migrations.resolve` parameter replaces `customResolver`. Explicit support for `wrap` and `nameFormatter` has been removed - these can be easily implemented in a `resolve` function. The constructor option `logging` is replaced by `logger` to allow for `warn` and `error` messages in future. NodeJS's global `console` object can be passed to this. To disable logging, replace `logging: false` with `logger: undefined`. The `Umzug#execute` method is removed. Use `Umzug#up` or `Umzug#down`. The options for `Umguz#up` and `Umzug#down` have changed: - `umzug.up({ to: 'some-name' })` and `umzug.down({ to: 'some-name' })` are still valid. - `umzug.up({ from: '...' })` and `umzug.down({ from: '...' })` are no longer supported. To run migrations out-of-order (which is not generally recommended), you can explicitly use `umzug.up({ migrations: ['...'] })` and `umzug.down({ migrations: ['...'] })`. - name matches must be exact. `umzug.up({ to: 'some-n' })` will no longer match a migration called `some-name`. - `umzug.down({ to: 0 })` is still valid but `umzug.up({ to: 0 })` is not. - `umzug.up({ migrations: ['m1', 'm2'] })` is still valid but the shorthand `umzug.up(['m1', 'm2'])` has been removed. - `umzug.down({ migrations: ['m1', 'm2'] })` is still valid but the shorthand `umzug.down(['m1', 'm2'])` has been removed. - `umzug.up({ migrations: ['m1', 'already-run'] })` will throw an error, if `already-run` is not found in the list of pending migrations. - `umzug.down({ migrations: ['m1', 'has-not-been-run'] })` will throw an error, if `has-not-been-run` is not found in the list of executed migrations. - `umzug.up({ migrations: ['m1', 'm2'], force: true })` will re-apply migrations `m1` and `m2` even if they've already been run. - `umzug.down({ migrations: ['m1', 'm2'], force: true })` will "revert" migrations `m1` and `m2` even if they've never been run. - `umzug.up({ migrations: ['m1', 'does-not-exist', 'm2'] })` will throw an error if the migration name is not found. Note that the error will be thrown and no migrations run unless _all_ migration names are found - whether or not `force: true` is added. The `context` parameter replaces `params`, and is passed in as a property to migration functions as an options object, alongs side `name` and `path`. This means the signature for migrations, which in v2 was `(context) => Promise<void>`, has changed slightly in v3, to `({ name, path, context }) => Promise<void>`. The `resolve` function can also be used to upgrade your umzug version to v3 when you have existing v2-compatible migrations: ```js const { Umzug } = require('umzug'); const umzug = new Umzug({ migrations: { glob: 'migrations/umzug-v2-format/*.js', resolve: ({name, path, context}) => { // Adjust the migration from the new signature to the v2 signature, making easier to upgrade to v3 const migration = require(path) return { up: async () => migration.up(context), down: async () => migration.down(context) } } }, context: sequelize.getQueryInterface(), logger: console, }); ``` Similarly, you no longer need `migrationSorting`, you can use `Umzug#extend` to manipulate migration lists directly: ```js const { Umzug } = require('umzug'); const umzug = new Umzug({ migrations: { glob: 'migrations/**/*.js' }, context: sequelize.getQueryInterface(), }) .extend(migrations => migrations.sort((a, b) => b.path.localeCompare(a.path))); ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I opened a ticket on knex-migrate issue tracker, but after investigating, I feel this issue is really more about umzug.
When
currently the library rejects the migration with the message: "Migration is not pending: %migration_name%".
I contend that this would be better as a success state. If I want a given migration to be run and it has already been run, then... mission accomplished? I think if this were the case, we could print "Migration already applied" and exit with a success status.
What do you think?
The text was updated successfully, but these errors were encountered: