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

Don't load container models from app folder in migration #7312

Merged

Conversation

lpichler
Copy link
Contributor

for possibility to use good_migration gem

related #6739

change from CONTAINER_TABLES toCONTAINER_MODELSwas needed because this command t.to_s.classify.constantize.update_all` is loading it from app/ folder even if we are defining these models in the migration

this changes have been made by this script

CONTAINER_TABLES = [:container_nodes, :container_projects, :container_services, :container_routes, :container_groups,
                    :container_replicators, :container_quotas, :container_builds, :container_build_pods,
                    :container_limits, :container_volumes]

type_column_exists = lambda { |t| ActiveRecord::Base.connection.column_exists?(t, :type) }
tables = CONTAINER_TABLES.map { |e| [e.to_s.classify.constantize, type_column_exists.call(e)] }
puts tables

tables.each do |t|
  print "\n"
  print "class #{t.first.to_s} < ActiveRecord::Base"
  if t.second
    print "\n"
    print "  self.inheritance_column = :_type_disabled # disable STI\n"
    print "end\n"
  else
    print "; end\n"
  end
end

print "CONTAINER_TABLES =["
CONTAINER_TABLES.each do |t|
  print "#{t.to_s.classify}, "
end

cc @jrafanie @Fryguy

@Fryguy
Copy link
Member

Fryguy commented Mar 16, 2016

@lpichler Is there a spec for this in spec/migrations?

@lpichler
Copy link
Contributor Author

@Fryguy there is no spec for it, so I will add it to this PR, thanks

@lpichler lpichler changed the title Don't load container models from app folder in migration [WIP] Don't load container models from app folder in migration Mar 16, 2016
@miq-bot miq-bot added the wip label Mar 16, 2016
@Fryguy
Copy link
Member

Fryguy commented Mar 16, 2016

/me is upset that got merged in the first place without a spec :(

self.inheritance_column = :_type_disabled # disable STI
end

class ContainerProject < ActiveRecord::Base; end
Copy link
Member

Choose a reason for hiding this comment

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

This was pre-rails 5, but in new migrations created with rails 5, I believe these should be < ApplicationRecord; end ... Right @matthewd?

Copy link
Contributor

Choose a reason for hiding this comment

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

No.

Part of the advantage of ApplicationRecord is that we can leave AR::Base unmolested, which is exactly what migrations want: a plain ordinary AR object with no app-code-of-the-day surprises.

Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

@matthewd That is really interesting and good to know! I didn't even think of that.

@lpichler lpichler force-pushed the dont_reference_containers_models_from_app branch from d9fbca2 to ed2e4d8 Compare March 17, 2016 10:26
@lpichler lpichler changed the title [WIP] Don't load container models from app folder in migration Don't load container models from app folder in migration Mar 17, 2016
@miq-bot miq-bot removed the wip label Mar 17, 2016
@lpichler
Copy link
Contributor Author

@jrafanie @Fryguy updated, specs added.

thanks @Ladas


def stub_for(table)
send(stub_name_for(table))
end
Copy link
Member

Choose a reason for hiding this comment

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

We probably shouldn't define these methods globally.

Copy link
Member

Choose a reason for hiding this comment

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

After you remove the tests for schema changes (columns dropped/added, see below), do we need these methods anymore? Maybe just do the work in-line if it's only a few places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it is only on few places now 👍

@lpichler lpichler force-pushed the dont_reference_containers_models_from_app branch from ed2e4d8 to ff856e2 Compare March 18, 2016 09:30
@miq-bot
Copy link
Member

miq-bot commented Mar 18, 2016

Checked commits lpichler/manageiq@4087231~...ff856e2 with ruby 2.2.3, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. 🍰

@lpichler
Copy link
Contributor Author

@jrafanie I removed testing of schema, now it looks simplier, thanks for your feedback!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 32.274% when pulling ff856e2 on lpichler:dont_reference_containers_models_from_app into c253de1 on ManageIQ:master.

@chessbyte chessbyte merged commit 58aaf50 into ManageIQ:master Mar 29, 2016
@chessbyte chessbyte added this to the Sprint 38 Ending Mar 28, 2016 milestone Mar 29, 2016
@lpichler lpichler deleted the dont_reference_containers_models_from_app branch April 4, 2016 17:35
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.

7 participants