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 #26536 - Possibility to change host loc/org via hammer #416

Merged
merged 1 commit into from Jul 30, 2019

Conversation

@ofedoren
Copy link
Member

commented Apr 4, 2019

No description provided.

@ofedoren ofedoren force-pushed the ofedoren:bug-26536-update-host-loc-org branch from e424bcf to 80ce0dc Apr 8, 2019
@ofedoren ofedoren changed the title [WIP]Fixes #26536 - Possibility to change host loc/org via hammer Fixes #26536 - Possibility to change host loc/org via hammer Apr 8, 2019
@ofedoren ofedoren force-pushed the ofedoren:bug-26536-update-host-loc-org branch from 80ce0dc to 7e2488d May 23, 2019
Copy link
Contributor

left a comment

Could you please explain how to update a host's organization by host name?

hammer host update --name myhost.example.com --organization-id 3 ????
@ofedoren

This comment has been minimized.

Copy link
Member Author

commented May 23, 2019

@akofink, try hammer host update --name myhost.example.com --organization-id id_of_hosts_current_organization --new-organization-id id_of_organization_to_be_assigned

@akofink

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

Great! This scenario isn't currently covered by tests afaict, right?

@ofedoren

This comment has been minimized.

Copy link
Member Author

commented May 23, 2019

@akofink, I'm afraid it isn't (or there are some minor oversights like in Katello/hammer-cli-katello#645). Also I've just noticed, when tried this command, that name passed as a parameter under host scope which isn't quite good since name should remain the same (command with those parameters should not change the host's name). It will not change the host's name since it is the same, but still not quite good...

Also, did the command I've written what you wanted? (I'm asking because maybe I'm missing something)

Copy link

left a comment

Hello
I tested moving hosts from one location to another, and one organisation to another, using IDs and names.

ACK

@mbacovsky

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

[test hammer]

@akofink

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

What's going on here?

@swadeley

This comment has been minimized.

Copy link

commented Jul 3, 2019

@ofedoren why no Assignees?

@ofedoren

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

@akofink, @mbacovsky currently isn't available :(

@swadeley, it's not so important in the scope of github. There is an assignee in the redmine issue.

@mbacovsky mbacovsky self-assigned this Jul 19, 2019
Copy link
Member

left a comment

Works for me. I like the new option builder. Inline I've left a suggestion to turn the resolving into an reusable option source. Makes sense?

@@ -77,6 +77,20 @@ def request_params
if action == :update
params['host']['compute_attributes']['volumes_attributes'] = nested_attributes(option_volume_list) unless option_volume_list.empty?
params['host']['interfaces_attributes'] = interfaces_attributes unless option_interface_list.empty?
new_location = option_new_location_name || option_new_location_title

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Jul 19, 2019

Member

Could this logic be extracted to the OptionSource so that it is resolved along with other options and reusable like the new OptionBuilder

@ofedoren ofedoren force-pushed the ofedoren:bug-26536-update-host-loc-org branch from 7e2488d to 63653cd Jul 25, 2019
@ofedoren

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

@mbacovsky, updated.

Now there are new option sources that will resolve any new_resource_name option.

@mbacovsky

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

@ofedoren, I like how the new option source is designed. Would you mind fixing the conflict before I do last round of testing?

@ofedoren ofedoren force-pushed the ofedoren:bug-26536-update-host-loc-org branch from 63653cd to afb8277 Jul 29, 2019
@ofedoren ofedoren force-pushed the ofedoren:bug-26536-update-host-loc-org branch from afb8277 to b260ec7 Jul 29, 2019
@ofedoren

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

@mbacovsky, done :)

@mbacovsky

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Thanks @ofedoren! 🍾

@mbacovsky mbacovsky merged commit 031522d into theforeman:master Jul 30, 2019
2 checks passed
2 checks passed
Redmine issues Valid issues
Details
hammer Build finished. 5341 tests run, 0 skipped, 0 failed.
Details
ofedoren added a commit to ofedoren/hammer-cli-foreman that referenced this pull request Aug 27, 2019
mbacovsky added a commit that referenced this pull request Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.