Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

typeorm migration:show #4173

Merged
merged 10 commits into from
May 30, 2019
Merged

typeorm migration:show #4173

merged 10 commits into from
May 30, 2019

Conversation

Juddling
Copy link
Contributor

@Juddling Juddling commented May 21, 2019

As proposed here: #3954

This PR adds a new command typeorm migration:show

This lists all migrations and shows whether they are applied or pending.

The command also returns an error code if there are unapplied migrations, so that it can easily be used in CI pipelines.

@ashbadger
Copy link

@Juddling could you also add this to the using-cli.md?

i took a stab:

Show migrations and whether they've been run or not

To show all migrations and whether they've been run or not use following command:

typeorm migration:show

This command also returns an error code if there are unapplied migrations.

@Juddling
Copy link
Contributor Author

Added, thank you @ashbadger

const executedMigration = executedMigrations.find(executedMigration => executedMigration.name === migration.name);

if (executedMigration) {
this.connection.logger.logSchemaBuild(` ✅ ${migration.name}`);
Copy link
Contributor

@Kononnable Kononnable May 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have much of experience in this, so I have to ask - will such characters be displayed in all/most of all terminals?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just had a dig through yarn's codebase and it looks like they only enable it for specific terminals, so I'll remove these emojis 😢 😞

https://github.com/yarnpkg/yarn/blob/d4805c302d9a02fd2a52cb4e02f9092b4689fbb1/src/cli/index.js#L116

  commander.option(
    '--emoji [bool]',
    'enable emoji in output',
    boolify,
    process.platform === 'darwin' ||
      process.env.TERM_PROGRAM === 'Hyper' ||
      process.env.TERM_PROGRAM === 'HyperTerm' ||
      process.env.TERM_PROGRAM === 'Terminus',
  );

default: "default",
describe: "Name of the connection on which run a query."
})
.option("transaction", {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably not necessary if we're just going to show migrations without running them.

@Juddling
Copy link
Contributor Author

@Kononnable thanks for the review, I've pushed changes with your recommendations

src/connection/Connection.ts Outdated Show resolved Hide resolved
let connections: Connection[];
before(async () => connections = await createTestingConnections({
migrations: [__dirname + "/migration/*.js"],
enabledDrivers: ["postgres"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we enable more drivers?

Copy link
Contributor

@vlapo vlapo May 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no driver specific changes. Run this test only for one driver is ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've enabled them all for now, if it adds too much of a time hit to the tests, we can drop to fewer drivers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll drop back down to just postgres then 👌

@pleerock pleerock merged commit 4d2fa07 into typeorm:master May 30, 2019
@pleerock
Copy link
Member

Thank you very much guys!

@Juddling Juddling deleted the show-migrations branch May 30, 2019 07:19
@ryanoboril
Copy link

ryanoboril commented Jun 28, 2019

Great work, thank you @Juddling!

@glen-84
Copy link

glen-84 commented Jan 31, 2021

@Juddling,

This makes absolutely no sense as the default behaviour. It is in no way an error for there to be unapplied migrations. Now, every time that this command is run manually by a user, they get 10 npm ERR! lines? That's just wrong.

If you need different behaviour in a CI environment, then add a CLI argument. A command run in a CI environment is usually only defined once, as opposed to users running this command many times manually. || 1 is not a solution.

@Juddling
Copy link
Contributor Author

@Juddling,

This makes absolutely no sense as the default behaviour. It is in no way an error for there to be unapplied migrations. Now, every time that this command is run manually by a user, they get 10 npm ERR! lines? That's just wrong.

If you need different behaviour in a CI environment, then add a CLI argument. A command run in a CI environment is usually only defined once, as opposed to users running this command many times manually. || 1 is not a solution.

Initial use case for this was to run in CI pre deploy, i.e. all code migrations should be run before new code version is deployed.

If you're running this locally I can see why the error code isn't useful. I'd suggest opening a github issue tagging me, outlining the issue (are you having any problems beyond the fact that you don't like seeing those err lines) and a proposed solution - baring in mind that people may depend on this command returning the error code so this is probably a breaking change

@alper
Copy link

alper commented Jul 17, 2024

Can we get this to also output the SQL?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants