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 #5556, #8246, #8299 as they are related #157

Merged
merged 3 commits into from
Dec 10, 2014

Conversation

tstrachota
Copy link
Member

Needs to be merged together with theforeman/hammer-cli#151

  • adds better error message when searchable options are missing
  • resolves params with array of names to ids
  • adds more name parameters to host and hostgroup create/update

@tstrachota
Copy link
Member Author

Related test update is here:
theforeman/hammer-tests#6

Check the test run:
http://ci.theforeman.org/job/systest_foreman_hammer_nightly/47/default/

end

attr_reader :name, :description

def plural_name
@plural_name || @name + 's'
Copy link
Member

Choose a reason for hiding this comment

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

Could ApipieBindings::Inflector.pluralize bring more flexibility?

@mbacovsky
Copy link
Member

hammer os update -h does not offer resolution for plurals

@tstrachota
Copy link
Member Author

The OS issue is covered by:
theforeman/foreman#1990

@tstrachota
Copy link
Member Author

@mbacovsky updated

@mbacovsky
Copy link
Member

The code looks good, all the three issues were fixed.
Only issue I found is missing --image in host create and --puppetclasses in hostgroup. Is that for any particular reason?

@tstrachota
Copy link
Member Author

@mbacovsky images are not searchable by anything else than ids. Puppet classes are tricky because the api returns hash class_name -> classes instead of array. It should be fixed now.

@mbacovsky
Copy link
Member

Interesting, from https://github.com/theforeman/foreman/blob/develop/app/models/image.rb#L16 it seems scoped search for images is defined on name and work for me in hammer:

hammer compute-resource image list --compute-resource-id  1 --search="name~Ce%"                                                                                                           
---|------------|------------------|----------|---------------------------------------------------------
ID | NAME       | OPERATING SYSTEM | USERNAME | UUID                                                    
---|------------|------------------|----------|---------------------------------------------------------
2  | CentOS 7.0 | CentOS 7.0       | root     | /home/VMs/CentOS-7-x86_64-GenericCloud-20140917_02.qcow2
---|------------|------------------|----------|---------------------------------------------------------

However this feature is not available in the UI.

Thanks for the puppetclass, seems we are almost there 🐇

@mbacovsky
Copy link
Member

10426188_852879981401061_6820265382025663263_n
Looks good now, lets merge it (when the tests are green)!

@mbacovsky
Copy link
Member

[test]

@domcleal
Copy link
Contributor

Maybe missing a require on "hammer_cli/apipie/option_definition.rb"?

/var/lib/workspace/workspace/test_hammer_cli_foreman_pull_request/ruby/1.8.7/lib/hammer_cli_foreman.rb:134: uninitialized constant HammerCLI::Apipie::OptionDefinition (HammerCLI::ModuleLoadingError)
    from /var/lib/workspace/workspace/test_hammer_cli_foreman_pull_request/ruby/1.8.7/test/unit/test_helper.rb:27:in `require'
    from /var/lib/workspace/workspace/test_hammer_cli_foreman_pull_request/ruby/1.8.7/test/unit/test_helper.rb:27
    from /var/lib/workspace/workspace/test_hammer_cli_foreman_pull_request/ruby/1.8.7/test/unit/fact_test.rb:1:in `require'
    from /var/lib/workspace/workspace/test_hammer_cli_foreman_pull_request/ruby/1.8.7/test/unit/fact_test.rb:1

@mbacovsky
Copy link
Member

@domcleal, I've merged the related part, with that the OptionDefinition should be defined.

mbacovsky added a commit that referenced this pull request Dec 10, 2014
Fixes #5556, #8246, #8299 as they are related
@mbacovsky mbacovsky merged commit ff0ade0 into theforeman:master Dec 10, 2014
domcleal pushed a commit to domcleal/foreman-bats that referenced this pull request Dec 10, 2014
Since theforeman/hammer-cli-foreman#157, the argument
has changed from --puppetclass-ids to --puppet-class-ids.

Looks up the argument name from help output as it's hard to judge which
version of Hammer is being used.
domcleal pushed a commit to domcleal/foreman-bats that referenced this pull request Dec 10, 2014
Since theforeman/hammer-cli-foreman#157, the argument
has changed from --puppetclass-ids to --puppet-class-ids.

Looks up the argument name from help output as it's hard to judge which
version of Hammer is being used.
@tstrachota tstrachota deleted the search_message branch July 14, 2015 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants