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 #33165 - Fix undefined resolver method with katello #8

Merged
merged 1 commit into from Jul 30, 2021

Conversation

xprazak2
Copy link
Contributor

We include the ResolverExtensions in ::HammerCLIForeman::IdResolver, but HammerCLIKatello::IdResolver actually subclasses it and we get:

defined? HammerCLIKatello::IdResolver #=> true
respond_to?(:create_search_options_without_katello_api) #=> false

So it seems like we do not need to check for katello methods inside foreman resolver.

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

Thanks, @xprazak2! Somehow I missed this case...

Although if we return create_search_options(options, api.resource(:ansible_roles), mode) to be the only option here it will break host update --ansible-roles command if hammer-cli-katello plugin is installed.

A better approach here will be to change the condition, please see inline.

@@ -1,11 +1,7 @@
module HammerCLIForemanAnsible
module ResolverExtension
def create_ansible_roles_search_options(options, mode = nil)
if defined? HammerCLIKatello::IdResolver
Copy link
Member

Choose a reason for hiding this comment

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

The easiest fix that comes to my mind is

if self.class.to_s == 'HammerCLIKatello::IdResolver'
  create_search_options_without_katello_api(options, api.resource(:ansible_roles), mode)
else
...

This should ensure that this method will be used if the current resolver comes from katello plugin. to_s is needed in case if katello plugin is not installed, so we don't crash with undefined constant HammerCLIKatello.

But I agree that this is still error-prone, so we should find a better way to deal with IdResolvers. I can imagine that different plugins will try to redefine a command to use its own resolver which will lead to similar problems...

@xprazak2
Copy link
Contributor Author

Updated.

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

@xprazak2, LGTM, thanks!

@ofedoren ofedoren merged commit d88b851 into theforeman:master Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants