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

Refactoring available flavors #12523

Merged
merged 2 commits into from
Nov 23, 2016
Merged

Refactoring available flavors #12523

merged 2 commits into from
Nov 23, 2016

Conversation

pkomanek
Copy link
Contributor

@pkomanek pkomanek commented Nov 9, 2016

Purpose or Intent

Refactoring the available_flavors method for the miq_ae_orchestration based on the issue bellow.

Links [Optional]

#12038

@pkomanek
Copy link
Contributor Author

pkomanek commented Nov 9, 2016

@miq-bot add_labels automate, enhancement, refactoring, wip, euwe/no

@pkomanek pkomanek changed the title Refactoring available flavors [WIP]Refactoring available flavors Nov 9, 2016
@mkanoor
Copy link
Contributor

mkanoor commented Nov 16, 2016

@pkomanek
Can the WIP be removed on this PR?

@mkanoor
Copy link
Contributor

mkanoor commented Nov 16, 2016

@pkomanek can the WIP be removed from this PR?

@pkomanek
Copy link
Contributor Author

@miq-bot remove_label wip

@pkomanek pkomanek changed the title [WIP]Refactoring available flavors Refactoring available flavors Nov 16, 2016
@miq-bot miq-bot removed the wip label Nov 16, 2016
{ 'service_template' => MiqAeMethodService::MiqAeServiceServiceTemplate.find(service_template.id) }
end
let(:ems) do
@flavor1 = FactoryGirl.create(:flavor, :name => 'flavor1')
Copy link
Contributor

Choose a reason for hiding this comment

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

@pkomanek could the flavor1 and flavor2 also be lets instead of instance variable

@pkomanek pkomanek closed this Nov 18, 2016
@pkomanek pkomanek reopened this Nov 18, 2016
@pkomanek pkomanek closed this Nov 18, 2016
@pkomanek pkomanek reopened this Nov 18, 2016
end

shared_examples_for "#having all flavors" do
let(:flavor1) { FactoryGirl.create(:flavor, :name => 'flavor1') }
Copy link
Contributor

@mkanoor mkanoor Nov 18, 2016

Choose a reason for hiding this comment

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

@pkomanek
These need to be service model and not the AR models, since Automate Methods work only with Service Models

@pkomanek
Copy link
Contributor Author

Hi Madhu,
I have a question about that. Is this change really necessary? I am asking,
because after the getting flavors from ae_service I get the objects of
MiqAeMethodService::MiqAeServiceFlavor class and that seems good to me.
What do you think about that?
Thanks,
Patrik.

PS: I get the same objects when I add these lines of code:

...................................................................................
let(:svc_model_flavor1) do
MiqAeMethodService::MiqAeServiceFlavor.find(flavor1.id)
end
let(:svc_model_flavor2) do
MiqAeMethodService::MiqAeServiceFlavor.find(flavor2.id)
end
let(:svc_model_orchestration_manager) do
MiqAeMethodService::MiqAeServiceExtManagementSystem.find(ems.id)
end

it "finds all the flavors and populates the list" do
  allow(svc_model_orchestration_manager).to receive(:flavors) {

[svc_model_flavor1, svc_model_flavor2] }

.........................................................................................
Am I doing something wrong?

On Fri, Nov 18, 2016 at 10:45 PM, Madhu Kanoor [email protected]
wrote:

@mkanoor commented on this pull request.

In spec/db/fixtures/ae_datastore/ManageIQ/Cloud/Orchestration/
Operations/Methods.class/methods/available_flavors_spec.rb
#12523 (review)
:

  • end
  • end
  • shared_examples_for "#having only default value" do
  • let(:default_desc_blank) { "" }
  • it "provides only default value to the flavor list" do
  •  described_class.new(ae_service).main
    
  •  expect(ae_service["values"]).to eq(nil => default_desc_blank)
    
  •  expect(ae_service["default_value"]).to be_nil
    
  • end
  • end
  • shared_examples_for "#having all flavors" do
  • let(:flavor1) { FactoryGirl.create(:flavor, :name => 'flavor1') }

@pkomanek https://github.com/pkomanek
These need to be service model and not the AR models


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#12523 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ASgblN3b2Y7ye6m0V5OvnStupZj92kYIks5q_hxsgaJpZM4KtcUC
.

@mkanoor
Copy link
Contributor

mkanoor commented Nov 21, 2016

@pkomanek
Everything in an automate method is service wrapped, the results might match because the same method is available in both the Model and the ServiceModel that is the reason why most of the tests work. But for all of our tests especially for automate we can only use service models. If you are converting an existing spec, which doesn't use Service Models but uses the real AR object, its because the Automate Engine is sitting between the Test and the Automate Method which wraps the AR object into service models

@pkomanek pkomanek closed this Nov 23, 2016
@pkomanek pkomanek reopened this Nov 23, 2016
@miq-bot
Copy link
Member

miq-bot commented Nov 23, 2016

Checked commits pkomanek/manageiq@e68519a~...ac6e182 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. 🏆

@mkanoor mkanoor merged commit 48e8352 into ManageIQ:master Nov 23, 2016
@mkanoor mkanoor added this to the Sprint 50 Ending Dec 5, 2016 milestone Nov 23, 2016
@pkomanek pkomanek deleted the refactoring_available_flavors branch November 24, 2016 09:18
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.

3 participants