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

Improving migration running & adding config. option for setting database user host #678

Closed
wants to merge 5 commits into from

Conversation

gabfr
Copy link

@gabfr gabfr commented Nov 27, 2018

Hi!

First of all, thanks for such a great package. I made a few changes that I believe that may be useful for somebody out there. They are:

  • I changed the way we run the tenant:migrate, because when I specify an array of migration paths, essentially I want them to run accordingly with their migration base names (the date of creation of the migration - we do not want to run into out-of-sequence-migration-running). The only problem is that I had to make a little change into the Laravel´s migrator to accept migration files paths instead of directory paths only (I intend to open a pull request on Laravel´s framework repository too).
  • Also, I created a configuration option called tenancy.db.tenant-database-user-host-permission to allow specifying a host when creating a new database user for a new site, enabling the multi-tenant to work with databases servers in another machines

That's it

@luceos
Copy link
Contributor

luceos commented Nov 28, 2018

Migration paths

Okay I need to think the migration thing over and we're going to wait on the reply on the framework first. We don't want to introduce features they already have or will have.

User host permission

  • Can you please not combine two features into one PR. Please remove the tenancy.db.tenant-database-user-host-permission code.
  • Can you clarify the need to use a different Database host for creating a user and its permissions? We already have the managing connection feature that allows you use any number of secondary system connections.

PRs with failing tests will not be merged.

@gabfr
Copy link
Author

gabfr commented Nov 29, 2018

@luceos the database host on the user permissions is not about where we will connect. It is about from where we connect. When you try to make a mysql connection from another machine on the same network that your mysql server machine is, without creating a user specifying its host or IP address from where its connecting.

You will get a permission denied, until you create a user specifying the IP or hostname from where it will be connecting.

Take a look at this stackoverflow: https://stackoverflow.com/a/16288118

Anyway, I’ll separate it in two PRs. Sorry for the mess.


And about the migration things, my PR got accepted in the Laravel Framework repository, so I believe its safe to say that we can merge this modification of how the migrations run.

@gabfr gabfr closed this Nov 29, 2018
@luceos
Copy link
Contributor

luceos commented Nov 29, 2018

And about the migration things, my PR got accepted in the Laravel Framework repository, so I believe its safe to say that we can merge this modification of how the migrations run.

Not quite sure, we also have the tenancy:run command which could be used to run tenant migrations now..

@fletch3555 @bkintanar ?

@gabfr
Copy link
Author

gabfr commented Nov 29, 2018

Yes, you do have the command to run tenancy migrations.

But have you ever tried specifying an array of migration paths on the tenancy.db.tenant-migrations-path configuration, and then running your console command tenancy:migrate?

Sorry for the lack of proper markdown here. I’m typing from my phone

@luceos
Copy link
Contributor

luceos commented Nov 30, 2018

have you ever tried specifying an array of migration paths

No because that's a pretty unusual scenario ;) I'm still waiting for an opinion from the others before I decide on this.

@bkintanar
Copy link
Member

I have less experience with using migration paths to comment on this matter.

@gabfr
Copy link
Author

gabfr commented Nov 30, 2018

@luceos haha, yes, it's because I use a module design pattern in my projects that separates the migrations of the modules. But you're right, it's a very unusual scenario

@luceos
Copy link
Contributor

luceos commented Dec 5, 2018

@fletch3555 ?

@luceos luceos requested review from luceos and fletch3555 December 5, 2018 09:36
@fletch3555
Copy link
Member

Updating the connection config's host value seems confusing, but it's local so it'll work. Perhaps just change it to a local variable instead.

Not sure how I feel about the config and env name. Seems overly verbose, but maybe I'm just nitpicking.

Not sure getMigrationPaths() belongs on the connection class. What was the reasoning for that vs something else?

I'm sure I could find more but I'm mobile, so reading the diff is a bit difficult.

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.

4 participants