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

Fixes #12233 - clean the interface cache only when becoming Managed::Host #59

Merged
merged 1 commit into from
Oct 21, 2015

Conversation

iNecas
Copy link
Member

@iNecas iNecas commented Oct 21, 2015

Without the patch, discovery reboot fails on

```NoMethodError: undefined method`drop_execution_interface_cache' for

@ares
Copy link
Member

ares commented Oct 21, 2015

maybe use became.respond_to?, I'm not sure what host types we'll ever see :-) Otherwise 👍

…Host

Without the patch, discovery reboot fails on

```
NoMethodError: undefined method `drop_execution_interface_cache' for #<Host::Discovered:0x007f6e28913ac8>
```
@iNecas iNecas force-pushed the fix-discovery-compatibility branch from ea0c06b to b9e063e Compare October 21, 2015 12:47
@stbenjam
Copy link
Member

Isn't this more a problem of us extending the descendants of Host::Base? I still don't think we need to do that, the UI doesn't allow executing jobs on discovery hosts. That might be worthwhile eventually in the Discovery case, but like @ares said, we don't know what host types we'll ever see, yet we assume now we should extend them all with our remote execution methods.

@iNecas
Copy link
Member Author

iNecas commented Oct 21, 2015

No, actually, this is hapending with your PR https://github.com/theforeman/foreman_remote_execution/pull/55/files#diff-24c9f52e5c8d0dc8ba4b5d2b7ec5de44R107, I noticed that, when I was testing the #55, So in master, it actually might work even now

@iNecas
Copy link
Member Author

iNecas commented Oct 21, 2015

@stbenjam I'm ok with closing this PR if you're going to address that in your PR (until now, I haven't realized that issue might be related to the #55)

@stbenjam
Copy link
Member

Yea @ares ran into a similar problem on another PR and we had a discussion about it, the outcome was to keep the descendants but remove Host::Base itself, but this is a good example of why not to do the descendants as well.

@ares is there a reason to keep the descendants you think? If so we can take this and I can change #55, I don't really have a strong opinion about it

@stbenjam
Copy link
Member

Per discussion on hangouts, APJ from me

@stbenjam
Copy link
Member

iNecas added a commit that referenced this pull request Oct 21, 2015
Fixes #12233 - clean the interface cache only when becoming Managed::Host
@iNecas iNecas merged commit 6635732 into theforeman:master Oct 21, 2015
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.

4 participants