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

method_missing for missing attr #96

Closed
wants to merge 2 commits into from

Conversation

jameswnl
Copy link
Contributor

#95

@jameswnl
Copy link
Contributor Author

@syncrou @bdunne this is a naive fix

@gmcculloug gmcculloug requested a review from bdunne July 11, 2017 15:03
@gmcculloug
Copy link
Contributor

@bzwei Please review

@bzwei
Copy link
Contributor

bzwei commented Jul 11, 2017

@jameswnl The idea is good. Need to add responding_to_missings?. Also please add a spec test.

@jameswnl jameswnl changed the title [WIP] method_missing for missing attr method_missing for missing attr Jul 11, 2017
@jameswnl
Copy link
Contributor Author

@miq-bot add_label bug

@miq-bot miq-bot added the bug label Jul 11, 2017
Copy link
Contributor

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

😱 I need to investigate further to figure out how we got here, but this feels pretty drastic.

@miq-bot
Copy link
Collaborator

miq-bot commented Jul 11, 2017

Checked commits jameswnl/ansible_tower_client_ruby@15ad288~...699b2f6 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

@bzwei
Copy link
Contributor

bzwei commented Jul 11, 2017

@bdunne The model class and methods are created while converting the first hash. If the second hash has some keys that do not appear in the first hash, you will get method not exist error if trying to access them. The fix tries to add new methods if possible.

@jameswnl
Copy link
Contributor Author

@bdunne yes please!

@jameswnl
Copy link
Contributor Author

@bdunne check my workaround fix for ManageIQ/manageiq-providers-ansible_tower#9 to get the spec to pass

@jameswnl
Copy link
Contributor Author

@miq-bot add_label blocker

@miq-bot
Copy link
Collaborator

miq-bot commented Jul 14, 2017

@jameswnl Cannot apply the following label because they are not recognized: blocker

@Fryguy
Copy link
Contributor

Fryguy commented Jul 19, 2017

I reviewed this with @jameswnl and @bdunne and what we discovered is (ignoring the current hack that's in the refresher_v2.yml) that every credential in the v2 yml does not have a domain and every credential in the v3 yaml does have a domain. This means that when talking to the same provider, the API is consistent and this issue doesn't exist.

However, the tests don't do that. One set of tests runs against a v2 API, and another set of tests run against v3. Since the models are shared, they are effectively creating test contamination depending on which one loads first and defines the class.

The only way this could happen in the real world is if 2 different towers are accessed in the same process and those towers are two different API versions. IMO, the only way to handle this is to create separate class sets based on the tower version. Those class sets can be subclasses of a common class to allow simpler .is_a? calls, if that's desired.

Credential
├─ CredentialV2
└─ CredentialV3

Within a kind, we're not sure if different kinds of the same type have different payloads. If they do, we may need to further subclass by kind.

Credential
├─ CredentialV2
│  ├─ CredentialVmwareV2
│  └─ CredentialAwsV2
└─ CredentialV3
   ├─ CredentialVmwareV3
   └─ CredentialAwsV3

In the above I just tacked on the V2/V3...it could also be done like one of the following

  • Credential::V2::Vmware < Credential
  • Credential::V2::Vmware < Credential::V2 < Credential

@Fryguy
Copy link
Contributor

Fryguy commented Jul 19, 2017

It seems this is sort of already done for JobTemplate:

def job_template_class
@job_template_class ||= begin
if Gem::Version.new(version).between?(Gem::Version.new(2), Gem::Version.new(3))
AnsibleTowerClient::JobTemplateV2
else
AnsibleTowerClient::JobTemplate
end
end
end
. Perhaps this can be done strategically for Credential to get over that bug, then generically for all classes in a follow up.

@jameswnl jameswnl closed this Jun 20, 2018
@miq-bot
Copy link
Collaborator

miq-bot commented Jun 20, 2018

@jameswnl Cannot apply the following label because they are not recognized: blocker

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants