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 #27343 - consider value not display name of compute_resource #432

Merged

Conversation

@kgaikwad
Copy link
Member

commented Jul 22, 2019

adding @shiramax and @ezr-ondrej into the loop

Provider friendly name of GCE compute resource is Google so compute attribute flow is broken for GCE.

Copy link
Contributor

left a comment

approved by me, @mbacovsky @ofedoren, can one of you review as well ?

Copy link
Member

left a comment

Looks good overall, just one comment inline that I couldn't resist.

@@ -37,7 +37,7 @@ class Create < HammerCLIForeman::CreateCommand

def request_params
params = super
compute_resource_name = HammerCLIForeman.record_to_common_format(HammerCLIForeman.foreman_resource(:compute_resources).call(:show, 'id' => options["option_compute_resource_id"] ))["provider_friendly_name"].downcase
compute_resource_name = HammerCLIForeman.record_to_common_format(HammerCLIForeman.foreman_resource(:compute_resources).call(:show, 'id' => options["option_compute_resource_id"] ))["provider"].downcase

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Jul 25, 2019

Member

All those identical lines are asking for DRYing. Could we have something like:

compute_resource_name = HammerCLIForeman.ComputeResources.resource_provider(options["option_compute_resource_id"])

I'd expect the function to live in hammer_cli_foreman/compute_resource/utils.rb

This comment has been minimized.

Copy link
@kgaikwad

kgaikwad Jul 25, 2019

Author Member

@mbacovsky,
To resolve this I need to require hammer_cli_foreman/compute_resource/utils file.

I am not sure that how this method call is being working.
When I debugged further using pry on host update then I wasn't able to see method get_host_compute_resource_id on ::HammerCLIForeman::ComputeResources. Am I missing something?

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Jul 25, 2019

Member

@kgaikwad you are right, we miss require on the cr utils file. We've lost it two weeks ago in here 7b59a14#diff-bbed5e1ba9432426e1430eeca619f8ba :(

This comment has been minimized.

Copy link
@kgaikwad

kgaikwad Jul 25, 2019

Author Member

Do you want me to add this require statement for host in this PR or separately?

This comment has been minimized.

Copy link
@ofedoren

ofedoren Jul 25, 2019

Member

There is an issue, so that fix will be in a separate PR.

This comment has been minimized.

Copy link
@kgaikwad

kgaikwad Jul 25, 2019

Author Member

Updated PR as per @mbacovsky suggested and created one #434 for host.

@kgaikwad kgaikwad force-pushed the kgaikwad:27343_fix_provider_in_compute_attr branch from 7615be3 to ac27ac9 Jul 25, 2019
@kgaikwad kgaikwad force-pushed the kgaikwad:27343_fix_provider_in_compute_attr branch from ac27ac9 to 73d03d5 Jul 25, 2019
Copy link
Member

left a comment

Haven't tested it with GCE provider, but codewise looks good. 👍

Copy link
Member

left a comment

Well done @kgaikwad! Thank you.

@mbacovsky mbacovsky merged commit b4a216c into theforeman:master Jul 26, 2019
2 checks passed
2 checks passed
Redmine issues Valid issues
Details
hammer Build finished. 5322 tests run, 0 skipped, 0 failed.
Details
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.