Skip to content
This repository has been archived by the owner on Jan 1, 2024. It is now read-only.

Do not load vanity playground if vanity is disabled. #173

Closed
wants to merge 1 commit into from
Closed

Do not load vanity playground if vanity is disabled. #173

wants to merge 1 commit into from

Conversation

adymo
Copy link

@adymo adymo commented Dec 18, 2013

This is a necessary addition to 1cdafa27d0
This way you can still run "rake db:migrate" for example even if experiment exists
and is vanity is used in the application code.

I don't quite like this solution, will appreciate any hints on how to fix this the right way.

This is a necessary addition to 1cdafa27d0
This way you can still run "rake db:migrate" for example even if experiment exists
and is vanity is used in the application code.
@phillbaker
Copy link
Collaborator

@adymo thanks for the pull request! Darn, I thought we had this licked in #81. This looks like a repeat of #77. It would help to get a little more info about the problem you're having, how could I reproduce the bug?

It looks like we need to avoid the call here:

Vanity.playground.load!
.

@adymo
Copy link
Author

adymo commented Dec 18, 2013

I don't have a test for this yet. I have a rails4 app that uses vanity (as a gem, loaded by bundler). The app has experiments set up. I recreated the database and ran "rake db:migrate". This failed because the initialization path goes through Vanity::Rails::load! (the code you've linked to). So it didn't matter that playground's constructor didn't connect to database. The load! should not be called at all when vanity is disabled.

@phillbaker
Copy link
Collaborator

Hm, yup, totally agree.

I think I'd lean toward something like:

# rails.rb
::Rails.configuration.after_initialize do
  Vanity.playground.load! if Vanity.playground.connected?
end

Thoughts? I'm curious if just short circuiting in load! could cause silent failures?

@adymo
Copy link
Author

adymo commented Dec 18, 2013

That looks like a better solution for me

phillbaker added a commit that referenced this pull request Dec 19, 2013
@phillbaker
Copy link
Collaborator

See 056a572 and 2ce41a0. Looks like there was also a bug in not falling through on the auto-connect portion.

@adymo
Copy link
Author

adymo commented Dec 19, 2013

Thanks, Phill!

@adymo adymo closed this Dec 19, 2013
phillbaker added a commit that referenced this pull request Dec 19, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants