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

Add another stub for Entitlement for preventing to load from app folder in migration #7729

Merged

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented Apr 5, 2016

#6739

this change is allowing to use https://github.com/testdouble/good-migrations
and prevents to load Entitlement model from app folder

@jrafanie @Fryguy @chrisarcand

@lpichler lpichler changed the title Add another stub for Entitlement for prevents loading from app folder in migration Add another stub for Entitlement for preventing to load from app folder in migration Apr 5, 2016
@jrafanie
Copy link
Member

jrafanie commented Apr 5, 2016

Good find @lpichler 👍

@chrisarcand
Copy link
Member

Wait the reason you added a second entitlement in this order is because
Rails auto loads the class via the association as soon as the model is
defined? Is that right?

On Tuesday, April 5, 2016, Joe Rafaniello [email protected] wrote:

Good find @lpichler https://github.com/lpichler [image: 👍]


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#7729 (comment)

Chris Arcand
@chrisarcand

@lpichler
Copy link
Contributor Author

lpichler commented Apr 6, 2016

@chrisarcand yes exaclty but this is valid when you are using definition with class

has_one :entitlement, :class_name => RemoveMiqUserRoleFromMiqGroups::Entitlement

but when you use it in quotes

has_one :entitlement, :class_name => 'RemoveMiqUserRoleFromMiqGroups::Entitlement'

it seems that is not loading the class 'RemoveMiqUserRoleFromMiqGroups::Entitlement' for this case even when I deleted model entitlement.rb => good_migration gem was silent and be db:migrate ran succesfully

(I am not sure what are exact differences in these cases when sql is generated or how impact it has, if any)

@jrafanie @Fryguy
so should I rather use it in quotas ? (then we can avoid multiple definition of classes in migrations for these cases)

@Fryguy
Copy link
Member

Fryguy commented Apr 6, 2016

I don't think Entitlement should be defined twice like that. I would think that all :class_name should be strings as that's how we do it in other migrations, and don't have a problem.

@chrisarcand
Copy link
Member

Ooooh I see. My fault entirely. Yes we should be using strings.

Aside, I hate these sorts of 'flexibility features' :disappointed_eyes:

@lpichler lpichler force-pushed the dont_load_entitlement_in_migration branch from d5fe5ee to 6b7afbb Compare April 7, 2016 08:43
@miq-bot
Copy link
Member

miq-bot commented Apr 7, 2016

Checked commit lpichler@6b7afbb with ruby 2.2.3, rubocop 0.37.2, and haml-lint 0.16.1
1 file checked, 0 offenses detected
Everything looks good. ⭐

@lpichler
Copy link
Contributor Author

lpichler commented Apr 7, 2016

ok, strings used, thanks guys 👍

@jrafanie
Copy link
Member

jrafanie commented Apr 7, 2016

👍

@jrafanie jrafanie merged commit 3c0b760 into ManageIQ:master Apr 7, 2016
@jrafanie jrafanie added this to the Sprint 39 Ending Apr 18, 2016 milestone Apr 7, 2016
@chrisarcand
Copy link
Member

👌

@lpichler lpichler deleted the dont_load_entitlement_in_migration branch April 7, 2016 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants