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 #21674 - Add abstraction for subcommand searching #342

Merged
merged 6 commits into from
Jun 6, 2019

Conversation

xprazak2
Copy link
Contributor

No description provided.

@theforeman-bot
Copy link
Member

@xprazak2, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).


This message was auto-generated by Foreman's prprocessor

@xprazak2 xprazak2 changed the title Fixes #21647 - Add abstraction for subcommand searching Fixes #21674 - Add abstraction for subcommand searching Nov 15, 2017
@xprazak2
Copy link
Contributor Author

xprazak2 commented Nov 15, 2017

Limitation:

hammer resource-a resource-bs --resource-a-id 13 --something some_value

if I create a subcommand in hammer_cli_plugin_a, where resourceA comes from pluginA and resourceB from core and pluginC extends resourceB with something that is not in a table of resourceB, then the search will not work, but --something will still be shown as acceptable option.

@ares
Copy link
Member

ares commented Dec 6, 2017

What's the status here? It blocks linked PR

@ares
Copy link
Member

ares commented Jan 19, 2018

Could someone please take a look? This blocks further hammer openscap improvements and it's been sitting untouched here more than 2 months.

@mbacovsky mbacovsky self-assigned this Feb 14, 2018
@ares
Copy link
Member

ares commented Mar 9, 2018

any progress?

@ares
Copy link
Member

ares commented Apr 6, 2018

ping @theforeman/hammer-admin

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.

I tried this abstraction in hammer-cli-foreman-ansible plugin and it worked fine for hammer host ansible-roles and hammer ansible roles hosts commands. I'd like to have it merged ASAP :)

lib/hammer_cli_foreman/commands.rb Outdated Show resolved Hide resolved
lib/hammer_cli_foreman/commands.rb Outdated Show resolved Hide resolved
@xprazak2
Copy link
Contributor Author

I made updates as suggested.

@ofedoren
Copy link
Member

@xprazak2, needs rebase

@xprazak2
Copy link
Contributor Author

xprazak2 commented Jul 2, 2018

Rebased.

@ares
Copy link
Member

ares commented Jul 13, 2018

Can we keep this moving? we should get this into 1.18 repos soon since the ansible hammer plugin is already there.

@ofedoren
Copy link
Member

@ares, just to be clear, I'm not using this approach in the latest version of ansible hammer plugin. By deafult it's waiting for theforeman/foreman_ansible#161 to be merged.

If this is merged before 161, then a new version of ansible hammer plugin needs to be released.

@ares
Copy link
Member

ares commented Jul 13, 2018

@ofedoren oh, based on this last comment Don't merge this for now, please. I guess there is another solution via #342.
I thought we don't need it anymore. Right now it seems we can't assigne roles to host/hostgroup. So lets pick whatever solution and make it working early next week. If you decide the PR in foreman_ansible is the way, let me know. I think it's now waiting on contributor but once updated, I can review, merge and do new release.

@ofedoren
Copy link
Member

ofedoren commented Jul 14, 2018

@ares are you sure we can't assign roles? I just checked via hammer host(group) create/update --ansible-role-ids 1,2,3 and it seems to be working.

Since there is no easier way to get list of host's roles than searching like hammer ansible roles list --search host=hostname I wanted to add simple commands like hammer host ansible-roles. This PR or 161 adds that functionality only and both have nothing in common with roles assigning.

@ares
Copy link
Member

ares commented Jul 17, 2018

You're right, I was playing with hammer host ansible-roles and always got a reply that server does not support this. But one can use the command you mentioned, thanks. Still it would be nice to get this PR merged :-)

Copy link
Member

@mbacovsky mbacovsky left a comment

Choose a reason for hiding this comment

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

@xprazak2 thanks for adding this and sorry for ignoring the PR for so long.

Looks good overall just see my two concerns inline.

@@ -279,6 +279,86 @@ def should_retrieve_all?

end

class ListSearchCommand < ListCommand
Copy link
Member

Choose a reason for hiding this comment

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

Could we have some more descriptive name? It took me while until I figured out what this class does. Would make AssociatedResourceListCommand more sense?

end

def self.default_search_options
option("--#{module_resource.singular_name}-id".tr('_', '-'), "ID", _("%s Id") % module_resource.singular_name)
Copy link
Member

Choose a reason for hiding this comment

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

I tried to convert host reports into descendant of this class. The resulting command looks good and easy to read. However in hammer we traditionally use --id and --name to refer to the "module resource" in this type of commands i.e. host reports --id and host reports --name. Would you mind to change the option names accordingly? It may make sense to add a mapping to change it if you have a use case for that.

@xprazak2
Copy link
Contributor Author

xprazak2 commented Aug 8, 2018

Updated, I changed the class name and the options --id, --name are used.

@ofedoren
Copy link
Member

ofedoren commented Aug 9, 2018

@xprazak2 could you please check the tests?

@xprazak2
Copy link
Contributor Author

I fixed tests, all green now.

@ares
Copy link
Member

ares commented Mar 15, 2019

any update here? @ofedoren / @mbacovsky

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.

Sorry, @xprazak2, @ares for the delay.

Looks good now (my inline comments are just for keeping code style the same as we have now or at least similar everywhere :) ), if @mbacovsky has nothing to add I'd merge it.

One more thing though: have you taken a look at

class AssociatedResourceListCommand < ListCommand
?

The purpose of these two looks very similar to me. Have you tried that command as a solution of the problem you had in other project?

P.S. I've run the test locally and only one failed: https://github.com/theforeman/hammer-cli-foreman/pull/342/files#diff-4c8a0b9258b2e44a8be27890bc78591eR156
There are no quotation marks in real output.

@@ -279,6 +279,86 @@ def should_retrieve_all?

end

class AssociatedListSearchCommand < ListCommand
def self.search_resource(res, action = :index)
resource res, action
Copy link
Member

Choose a reason for hiding this comment

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

Usually if we have method called with arguments, we use parenthesis.


def validate_options
super
validator.any("option_name".to_sym, "option_id".to_sym).required
Copy link
Member

Choose a reason for hiding this comment

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

Why use .to_sym instead of just passing a symbol right away? Like here:

validator.any(:option_domain_name, :option_domain_id).required

end

def self.search_options_mapping(mapping = {})
{ "name" => module_resource.singular_name,
Copy link
Member

Choose a reason for hiding this comment

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

Rubocop style nitpick :)

Doesn't look better?

{
  'name' => module_resource.singular_name,
  'id' => "#{module_resource.singular_name}_id"
}.merge(mapping)

@ofedoren
Copy link
Member

[test]

Ondrej Prazak added 2 commits April 12, 2019 10:40

Verified

This commit was signed with the committer’s verified signature. The key has expired.
kylekurz Kyle Kurz
@xprazak2
Copy link
Contributor Author

Rebased.

@ofedoren, I was looking at AssociatedResourceListCommand. The purpose is indeed similar, however from looking at FactsCommand, it seems like it expects nested routes:

2019-04-12T09:05:12 [I|app|6fc3dd11] Started GET "/api/hosts/38/facts?page=1&per_page=1000&id=38" for ::1 at 2019-04-12 09:05:12 +0000
2019-04-12T09:05:12 [I|app|6fc3dd11] Processing by Api::V2::FactValuesController#index as JSON
2019-04-12T09:05:12 [I|app|6fc3dd11]   Parameters: {"page"=>"1", "per_page"=>"1000", "id"=>"38", "apiv"=>"v2", "host_id"=>"38", "fact_value"=>{}}

Policy and host are not nested so the policy_id is never used and I get all the hosts, but I only want the hosts associated to policy:

2019-04-12T09:05:22 [I|app|f3281aa6] Started GET "/api/hosts?page=1&per_page=1000&id=1&policy_id=1" for ::1 at 2019-04-12 09:05:22 +0000
2019-04-12T09:05:22 [I|app|f3281aa6] Processing by Api::V2::HostsController#index as JSON
2019-04-12T09:05:22 [I|app|f3281aa6]   Parameters: {"page"=>"1", "per_page"=>"1000", "id"=>"1", "policy_id"=>"1", "apiv"=>"v2", "host"=>{}}

I need to specify a search somehow, so that only the associated hosts are returned:

2019-04-12T09:17:01 [I|app|a4861261] Started GET "/api/hosts?page=1&per_page=1000&id=1&search=compliance_policy_id%3D1" for ::1 at 2019-04-12 09:17:01 +0000
2019-04-12T09:17:01 [I|app|a4861261] Processing by Api::V2::HostsController#index as JSON
2019-04-12T09:17:01 [I|app|a4861261]   Parameters: {"page"=>"1", "per_page"=>"1000", "id"=>"1", "search"=>"compliance_policy_id=1", "apiv"=>"v2", "host"=>{}}

@mbacovsky, I had to revert the option naming back, having just --id works fine, but --name causes problems. The main reason is that the name is of a parent resource, but hammer uses it to fetch id of a child.

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, you might want to return --id and --name options, because there is a way to omit id resolving that caused problems you described:

If you don't need to resolve id by name (I presume you want to skip it since you're using search by name instead), you can remove the SelfParam option source, redefining the option_sources method like

def option_sources
  sources = super
  sources.find_by_name('IdResolution').insert_relative(
    :replace,
    'SelfParam',
     HammerCLI::Options::Sources::Base.new
  )
  sources
end

Otherwise, it seems to be working pretty well :)

end
end
comm = DomainOuter::HostsCommand.new("", { :adapter => :csv, :interactive => false })
out, err = capture_io { comm.run(["--host-id=5"]) }
Copy link
Member

Choose a reason for hiding this comment

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

--host-id is wrong parameter in this case (it just doesn't exist, since parent command has domains resource, so it should be rather --domain-id as you designed).

end
comm = DomainOuter::HostsCommand.new("", { :adapter => :csv, :interactive => false })
out, err = capture_io { comm.run(["--host-id=5"]) }
out.must_equal "Id,Name,Operating System,Host Group,IP,MAC\n2,random-host,\"\",\"\",192.168.100.112,6e:4b:3c:2c:8a:0a\n"
Copy link
Member

Choose a reason for hiding this comment

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

There are no quotes in real output, so test will fail.

@xprazak2
Copy link
Contributor Author

xprazak2 commented Jun 5, 2019

I pushed changes, thanks for suggestions @ofedoren!

Copy link
Member

@mbacovsky mbacovsky 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, my concerns were addressed. I leave the squash and merge exercise up to @ofedoren when he is happy with the PR.

@ofedoren
Copy link
Member

ofedoren commented Jun 6, 2019

Thanks for being patient, @xprazak2! Finally we are merging it :)

@ofedoren ofedoren merged commit 27286f9 into theforeman:master Jun 6, 2019
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.

None yet

5 participants