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

Depend on information_schema for MyXQL driver #80

Merged
merged 10 commits into from
May 24, 2021

Conversation

HammamSamara
Copy link
Contributor

@HammamSamara HammamSamara commented May 4, 2021

This pull request remove the dependency on the tenants table in favor of the existing information_schema.schemata table, which brings MySQL tenancy creation flow closer to the postgres flow.

@coveralls
Copy link

coveralls commented May 4, 2021

Coverage Status

Coverage increased (+0.01%) to 99.515% when pulling 41ff67a on Userpilot:drop-tenants-table into bdf19bb on ateliware:master.

Copy link
Contributor

@kelvinst kelvinst left a comment

Choose a reason for hiding this comment

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

Sorry it took me so long to review, I left you a comment, and I still need to test it manually on my side, to be sure backwards compatibility is maintained.

lib/triplex/config.ex Outdated Show resolved Hide resolved
lib/triplex/config.ex Outdated Show resolved Hide resolved
@HammamSamara
Copy link
Contributor Author

Sorry it took me so long to review, I left you a comment, and I still need to test it manually on my side, to be sure backwards compatibility is maintained.

Thank you for taking the time reviewing the change set. As you may anticipate it, getting rid of the tenants table might be a breaking change for some that needs to be reflected on the repo mix versioning.

@kelvinst
Copy link
Contributor

Yeah, but I think we could make it backward compatible by rollbacking the tenants_table config and using the old code for when the config exists. So we should just change the default to information_schema and notify users that when upgrading, they should either run a migration to send all the tenants from the tenants table to the information_schema or set the tenants_table config and keep using the same as before.

@HammamSamara
Copy link
Contributor Author

Rolling back most of the changes to keep alignment with the tenants table, while using information_schema as the default config.
Users need to set tenants_table as :tenants like I do for tests to keep it backward compatible:

config :triplex, tenant_table: :tenants

@HammamSamara
Copy link
Contributor Author

After a quick thought about compatibility and for safer merge, I am setting the default behavior to work with the :tenants table so all users segments don't need to change anything now (100% backward compatible).

If anyone (like me) wishes to not work with :tenant table and generates the associate migrations, then they can just configure Triplex to work with the "information_schema.schemata" as mentioned in the docs.

config :triplex, tenant_table: :"information_schema.schemata"

@kelvinst
Copy link
Contributor

Pretty cool! Awesome work, thanks! And sorry for all the back and forth. Merging it to master, will release a new version of triplex with your changes soon.

@kelvinst
Copy link
Contributor

@HammamSamara one last request: could you open a new PR changing the default behavior to use the information schema? I think that's a sane default as it's simpler than having to add a new table. We will then release that other one on 2.0

@kelvinst kelvinst merged commit a7e2dd4 into ateliware:master May 24, 2021
@HammamSamara
Copy link
Contributor Author

PR opened here #81

@HammamSamara HammamSamara deleted the drop-tenants-table branch May 28, 2021 23:44
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.

3 participants