graphile / migrate

Opinionated SQL-powered productive roll-forward migration tool for PostgreSQL.
MIT License
752 stars 58 forks source link

Add option to skip graphile migrate schema create #43

Closed oost closed 4 years ago

oost commented 4 years ago

Certain environment don't allow users to create schemas using DDL.

Adding --skip-own-schema flag allows user to specify that the graphite_migrations schema has already been created (as well as the tables).

benjie commented 4 years ago

Hey; thanks for the PR! This is a good idea. I’m not sure it should be a CLI flag, but rather a setting in gmrc; what do you think?

Whether the option comes from CLI flag or gmrc file, it should be passed through the tree using parsedSettings rather than adding extra parameters - please refactor to enable this, it should reduce the size of the diff somewhat.

Thanks again!

oost commented 4 years ago

Makes sense. I even to put in in gmrc given that it is almost always going to be an immutable feature of the platform (just like the connection string).

benjie commented 4 years ago

@oost You've added your .env file; I recommend you remove it, rebase to remove it from history, and cycle any secrets since they aren't any more. Also feel free to add .env to .gitignore to prevent this happening again.

benjie commented 4 years ago

Also the .gmrc.

This is a simple feature, and the implementation is suitably simple, so there's a good chance this'll be merged. I need to think about the name of the option, I'm not sure skipOwnSchema is the right name, but I don't have an alternative currently. We'll also want some documentation added to the README, but again that can wait until we know what the option name is. So leave it with me for a few days.

oost commented 4 years ago

Indeed, rookie mistake.

Added those files to .gitignore and added a bullet in the README section.

Agree, skipOwnSchema is a bit verbose and not very description. Will try to think of something else.

Also, was considering whether skipping only the CREATE SCHEMA part or the whole schema + table but went for the latter. In the environment I would like to use, schema privileges are not granted to users but creating the schema and tables is a one-off effort anyway.

oost commented 4 years ago

How about calling it createGraphileSchema, defaulting to true and skipping on false?

benjie commented 4 years ago

I was thinking similar, like runOwnMigrations: false but that’s potentially confusing. manageSchema: false maybe? It couldn’t just be Graphile, it would have to be GraphileMigrate to avoid confusion. How about trackMigrations: false? No... we’d still need to track them. manageTrackingSchema: false?

If you’ve created a compatible schema then presumably Graphile Migrate doesn’t try and migrate it anyway?

oost commented 4 years ago

Agree, should reference migrate (or graphile-migrate).

How about:

benjie commented 4 years ago

Lets go with manageGraphileMigrateSchema; it’s pretty clear what it does I think. We can always come up with an abbreviation later.

oost commented 4 years ago

Great. I've refactored the code with that name.

oost commented 4 years ago

Just added the checks. Let me know if this works for you.

benjie commented 4 years ago

Added some tests and did some refactoring in #45

benjie commented 4 years ago

This is now released as v0.0.18