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.

Sign up for GitHub

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

Jump to bottom

Avoid early connection #39

Merged
merged 1 commit into from
May 18, 2020
Merged

Avoid early connection #39

merged 1 commit into from
May 18, 2020

Conversation

fsateler
Copy link
Contributor

Fixes influitive/apartment#607 in a more general way than #16

@rpbaltazar
Copy link
Contributor

The code seems to make sense to me, but it breaks all the specs. can you please take a look a it?

@fsateler fsateler force-pushed the feature/avoid-early-connection branch from 45ce2ab to 6789fec Compare April 13, 2020 18:49
@fsateler
Copy link
Contributor Author

I could not understand the failure completely, but it appears to be related to recursive calls to connection. I have solved the problem by ensuring we only init once per Reload.

@rpbaltazar
Copy link
Contributor

I'm opening a new PR #50 with the changes because there has been no reply. I'll look at it and see if i can fix the issue.

@fsateler
Copy link
Contributor Author

Sorry, I dropped the ball. The problem seems to be that init does not invoke process_excluded_models. Maybe it should? I have added a switch to the current tenant, lets see if the tests passes. Not sure if the fix makes sense.

@fsateler
Avoid getting a connection to the database before it's necessary
256de6f 
We init the first time we request a connection. On code reloads, we
"forget" about our init state and we call it again.

After init, ensure the search path is set properly
@fsateler fsateler force-pushed the feature/avoid-early-connection branch from b94862e to 256de6f Compare May 14, 2020 22:19
@fsateler
Copy link
Contributor Author

OK, so it seems the problem was instead I was relying on an instance variable in ActiveRecord::Base. Switching that to a variable in Apartment::Tenant fixed most of the tests. To fix the remaining ones, I had to move init to the adapters so that the postgres adapter can set the schema search path. I don't know why that is needed but it doesn't seem wrong either.

@fsateler
Copy link
Contributor Author

Rubocop failure appears to be configuration-related.

@rpbaltazar
Copy link
Contributor

Thank you. Will get it merged and will start testing against our test suite, as well as out staging env

@rpbaltazar rpbaltazar merged commit 26fc746 into rails-on-services:development May 18, 2020
@rpbaltazar rpbaltazar mentioned this pull request May 18, 2020
Closed
@fsateler fsateler deleted the feature/avoid-early-connection branch May 19, 2020 14:34
@rpbaltazar rpbaltazar mentioned this pull request Jun 1, 2020
Merged
rpbaltazar added a commit that referenced this pull request Jun 2, 2020
@rpbaltazar
Merge pull request #59 from rails-on-services/development
1dd198d 
Prepare Release - 2.6.1

# Enhancements

- N/a

# Bugfixes

- [Resolves influitive#607] Avoid early connection
  - #39
  - #53
  - #51
- [Resolves #52] Rake db:setup tries to seed non existent tenant - #54
- [Resolves #56] DB rollback uses second last migration - #57

# Chores

- N/a
@fsateler fsateler mentioned this pull request Oct 6, 2020
Open
@lunks lunks mentioned this pull request Oct 26, 2020
Merged
@rpbaltazar rpbaltazar mentioned this pull request Dec 16, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rpbaltazar rpbaltazar rpbaltazar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Establishes connection during application boot
2 participants
@fsateler @rpbaltazar