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

Fix fallouts from ContainerTemplate STI #81

Merged
merged 2 commits into from
Aug 3, 2017

Conversation

cben
Copy link
Contributor

@cben cben commented Aug 3, 2017

I assumed in this code the model used when defining these collections is same model as the parent for which we are looking them up, which is wrong given STI.
This fixes errors in manageiq-providers-openshift repo:

    can't save: missing @inv_collections[[:custom_attributes_for, "ContainerTemplate", "labels"]]
    can't save: missing @inv_collections[[:custom_attributes_for, "ManageIQ::Providers::Kubernetes::ContainerManager::ContainerTemplate", "labels"]]

following ManageIQ/manageiq-providers-openshift#28 and related PRs.

I'm still seeing other errors in openshift specs locally, but this gets us closer to green...

@lfu @moolitayer @Ladas please review.

I was assuming the model used when defining these collections
is same model as the `parent` for which we are looking them up,
which is wrong given STI.
This fixes errors in manageiq-providers-openshift repo:
    can't save: missing @inv_collections[[:custom_attributes_for, "ContainerTemplate", "labels"]]
    can't save: missing @inv_collections[[:custom_attributes_for, "ManageIQ::Providers::Kubernetes::ContainerManager::ContainerTemplate", "labels"]]
following ManageIQ/manageiq-providers-openshift#28
and related PRs.
@@ -418,7 +418,7 @@ def get_pods_graph(inv)

# polymorphic, relation disambiguates parent
def get_container_conditions_graph(parent, hashes)
model_name = parent.inventory_collection.model_class.name
model_name = parent.inventory_collection.model_class.base_class.name

Choose a reason for hiding this comment

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

we don't need base class here right? (since the classes use in collections are already basic)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right we shouldn't need it, but experimentally we do :-\ Without it, in ManageIQ/manageiq-providers-openshift#28 refresher specs,
I get weird Invalid single-table inheritance type: ManageIQ::Providers::Openshift::ContainerManager::ContainerTemplate is not a subclass of ManageIQ::Providers::ContainerManager::ContainerTemplate

This parent-sniffing magic is too fragile, I'm gonna replace it with something more explicit (plus give inventory collections unique .names for debugging). But for now let's get back to green.

…erTemplate

Autoload didn't happen, const was silently resolving to
ManageIQ::Providers::ContainerManager::ContainerTemplate,
very confusing.
We want to explicitly load everything anyway.
@cben cben changed the title Use STI base_class to identify polymorphic collections Fix fallouts from ContainerTemplate STI Aug 3, 2017
@miq-bot
Copy link
Member

miq-bot commented Aug 3, 2017

Checked commits cben/manageiq-providers-kubernetes@7419a05~...8baff02 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

@cben
Copy link
Contributor Author

cben commented Aug 3, 2017

Added missing require_nested (see https://github.com/ManageIQ/manageiq/pull/15523/files#r131175697 for interesting way in which autoloading failed; doesn't matter much, we want explicit load all files)

Copy link

@moolitayer moolitayer left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Fryguy Fryguy merged commit 9980dd9 into ManageIQ:master Aug 3, 2017
@Fryguy Fryguy added the bug label Aug 3, 2017
@Fryguy Fryguy added this to the Sprint 66 Ending Aug 7, 2017 milestone Aug 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants