Check that the Embedded Ansible role is on #15045

syncrou · 2017-05-09T19:21:45Z

Raise an error if an attempt at raw_connect is made and the role is not enabled
Continue on as normal with the raw_connect if the role is enabled

https://bugzilla.redhat.com/show_bug.cgi?id=1448186

syncrou · 2017-05-09T19:22:37Z

@miq-bot add_label wip, bug

syncrou · 2017-05-09T19:23:32Z

@bdunne, @carbonin - Left this as a WIP to make sure I was taking the right path. Certainly wouldn't mind your thoughts.

carbonin

A few spec suggestions.

carbonin · 2017-05-09T19:29:08Z

spec/models/manageiq/providers/embedded_ansible/provider_spec.rb

@@ -35,5 +35,17 @@
        end
      end
    end
+
+    context "Embedded Ansible role" do
+      let(:miq_server) { FactoryGirl.create(:miq_server) }


let is lazy evaluated so this won't actually get run.

I think a better option would be to do this in a before and actually set up the role on that server for the success case.

Then you can get rid of the allow(described_class) as well.

carbonin · 2017-05-09T19:31:40Z

spec/support/ansible_shared/provider.rb

@@ -3,15 +3,18 @@
 shared_examples_for "ansible provider" do
  describe "#connect" do
    let(:attrs) { {:username => "admin", :password => "smartvm", :verify_ssl => OpenSSL::SSL::VERIFY_PEER} }
+    let(:miq_server) { FactoryGirl.create(:miq_server) }


These changes are embedded specific, so I feel like they belong in the embedded provider spec rather than in the shared examples.

carbonin · 2017-05-09T19:33:14Z

Yeah, sorry I just jumped in when I saw the notification @syncrou

This is what we talked about so the implementation looks good to me. I just had spec comments.

syncrou · 2017-05-09T21:26:05Z

@miq-bot remove_label wip

1. Raise an error if an attempt at raw_connect is made if the role is not enabled 2. Continue on as normal with the connect if the role is enabled https://bugzilla.redhat.com/show_bug.cgi?id=1448186

gmcculloug · 2017-05-10T18:27:51Z

@carbonin Can you quickly review after updates to get tests passing?

carbonin

Yup, I'm still okay with this.

bdunne · 2017-05-10T18:40:45Z

spec/support/evm_spec_helper.rb

@@ -21,6 +21,12 @@ def self.vmware_nested_folders(ems)
    end
  end

+  def self.assign_embedded_ansible_role(server = nil)
+    miq_server = server.nil? ? local_miq_server : server


could simplify to miq_server = server || local_miq_server
or

def self.assign_embedded_ansible_role(miq_server = nil) miq_server ||= local_miq_server ...

bdunne · 2017-05-10T18:42:34Z

app/models/manageiq/providers/embedded_ansible/provider.rb

@@ -8,4 +8,14 @@ class ManageIQ::Providers::EmbeddedAnsible::Provider < ::Provider
          :class_name  => "ManageIQ::Providers::EmbeddedAnsible::AutomationManager",
          :dependent   => :destroy,
          :autosave    => true
+
+  def self.raw_connect(base_url, username, password, verify_ssl)
+    raise StandardError, 'Embedded ansible is disabled' unless role_enabled?


Maybe prefer the positive case?

def self.raw_connect(base_url, username, password, verify_ssl) return super if role_enabled? raise... end

https://bugzilla.redhat.com/show_bug.cgi?id=1448186

miq-bot · 2017-05-10T19:03:02Z

Checked commits syncrou/manageiq@27dc7f8~...6b99c98 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
8 files checked, 0 offenses detected
Everything looks fine. 👍

bdunne

👍 LGTM

syncrou · 2017-05-10T20:27:17Z

@miq-bot add_label blocker

…nection Check that the Embedded Ansible role is on (cherry picked from commit dcf61f4) https://bugzilla.redhat.com/show_bug.cgi?id=1449846

simaishi · 2017-05-10T21:38:02Z

Fine backport details:

$ git log -1
commit 262fb8c7c05eb692da6f6a127d3587e8b231b8e8
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Wed May 10 15:16:20 2017 -0400

    Merge pull request #15045 from syncrou/override_embedded_provider_connection
    
    Check that the Embedded Ansible role is on
    (cherry picked from commit dcf61f46181adf1dacc1e5e3eff2af4aa4a88240)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1449846

miq-bot changed the title ~~Check the Embedded Ansible role is on~~ [WIP] Check the Embedded Ansible role is on May 9, 2017

miq-bot added bug wip labels May 9, 2017

miq-bot assigned gmcculloug May 9, 2017

carbonin suggested changes May 9, 2017

View reviewed changes

gmcculloug added the fine/yes label May 9, 2017

syncrou force-pushed the override_embedded_provider_connection branch from 1d06e17 to f691103 Compare May 9, 2017 21:25

miq-bot changed the title ~~[WIP] Check the Embedded Ansible role is on~~ Check the Embedded Ansible role is on May 9, 2017

miq-bot removed the wip label May 9, 2017

syncrou force-pushed the override_embedded_provider_connection branch from f691103 to a8eb279 Compare May 9, 2017 21:33

carbonin approved these changes May 9, 2017

View reviewed changes

carbonin requested a review from bdunne May 9, 2017 21:44

syncrou changed the title ~~Check the Embedded Ansible role is on~~ Check that the Embedded Ansible role is on May 9, 2017

Check the Embedded Ansible role is on

27dc7f8

1. Raise an error if an attempt at raw_connect is made if the role is not enabled 2. Continue on as normal with the connect if the role is enabled https://bugzilla.redhat.com/show_bug.cgi?id=1448186

syncrou force-pushed the override_embedded_provider_connection branch from a8eb279 to 621a7ca Compare May 10, 2017 18:05

carbonin approved these changes May 10, 2017

View reviewed changes

bdunne reviewed May 10, 2017

View reviewed changes

Modify specs to set the embedded role

6b99c98

https://bugzilla.redhat.com/show_bug.cgi?id=1448186

syncrou force-pushed the override_embedded_provider_connection branch from 621a7ca to 6b99c98 Compare May 10, 2017 19:00

bdunne approved these changes May 10, 2017

View reviewed changes

bdunne merged commit dcf61f4 into ManageIQ:master May 10, 2017

bdunne added the providers/ansible_tower label May 10, 2017

bdunne added this to the Sprint 61 Ending May 22, 2017 milestone May 10, 2017

syncrou deleted the override_embedded_provider_connection branch May 10, 2017 19:17

miq-bot added the blocker label May 10, 2017

simaishi added fine/backported and removed fine/yes labels May 10, 2017

carbonin mentioned this pull request May 15, 2017

Use MiqRegion#role_active? method over private method on Provider #15094

Merged

Fryguy added the providers/automation label Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check that the Embedded Ansible role is on #15045

Check that the Embedded Ansible role is on #15045

syncrou commented May 9, 2017 •

edited

Loading

syncrou commented May 9, 2017

syncrou commented May 9, 2017

carbonin left a comment

carbonin May 9, 2017

carbonin May 9, 2017

carbonin May 9, 2017

carbonin commented May 9, 2017

syncrou commented May 9, 2017

gmcculloug commented May 10, 2017

carbonin left a comment

bdunne May 10, 2017

bdunne May 10, 2017

miq-bot commented May 10, 2017

bdunne left a comment

syncrou commented May 10, 2017

simaishi commented May 10, 2017

Check that the Embedded Ansible role is on #15045

Check that the Embedded Ansible role is on #15045

Conversation

syncrou commented May 9, 2017 • edited Loading

syncrou commented May 9, 2017

syncrou commented May 9, 2017

carbonin left a comment

Choose a reason for hiding this comment

carbonin May 9, 2017

Choose a reason for hiding this comment

carbonin May 9, 2017

Choose a reason for hiding this comment

carbonin May 9, 2017

Choose a reason for hiding this comment

carbonin commented May 9, 2017

syncrou commented May 9, 2017

gmcculloug commented May 10, 2017

carbonin left a comment

Choose a reason for hiding this comment

bdunne May 10, 2017

Choose a reason for hiding this comment

bdunne May 10, 2017

Choose a reason for hiding this comment

miq-bot commented May 10, 2017

bdunne left a comment

Choose a reason for hiding this comment

syncrou commented May 10, 2017

simaishi commented May 10, 2017

syncrou commented May 9, 2017 •

edited

Loading