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

[wip]Upgrade Ovirt Cluster through Ansibel #304

Closed

Conversation

borod108
Copy link
Contributor

Add method to upgrade cluster using ovirt-ansible-cluster-upgrade role.

Depends on: ManageIQ/manageiq#18108

@borod108 borod108 added the wip label Oct 29, 2018
@borod108 borod108 changed the title Upgrade Ovirt Cluster through Ansibel [wip] Upgrade Ovirt Cluster through Ansibel Oct 29, 2018
engine_user: connect_options[:username],
engine_password: connect_options[:password],
cluster_name: name,
hostname: "localhost"
Copy link
Contributor

Choose a reason for hiding this comment

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

localhost?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it runs on local host. the target is defined in the engine_url.

engine_url: url,
engine_user: connect_options[:username],
engine_password: connect_options[:password],
cluster_name: name,
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see name being defined. Is it defined in super class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, in EmsCluster

@masayag
Copy link
Contributor

masayag commented Oct 30, 2018

@borod108 typo in header - Ansibel

@borod108 borod108 closed this Oct 31, 2018
@borod108 borod108 reopened this Oct 31, 2018
@borod108 borod108 force-pushed the rfe/ansible_cluster_upgrade branch 2 times, most recently from 00431af to 81a6b20 Compare November 3, 2018 10:10
@borod108 borod108 removed the wip label Nov 3, 2018
@borod108 borod108 changed the title [wip] Upgrade Ovirt Cluster through Ansibel Upgrade Ovirt Cluster through Ansibel Nov 3, 2018
@borod108
Copy link
Contributor Author

borod108 commented Nov 3, 2018

@pkliczewski @masayag will you please review and merge?

@borod108 borod108 force-pushed the rfe/ansible_cluster_upgrade branch from 81a6b20 to 78284d6 Compare November 3, 2018 10:11
@masayag masayag closed this Nov 3, 2018
@masayag masayag reopened this Nov 3, 2018
@pkliczewski
Copy link
Contributor

@borod108 Please make sure that Travis is happy.

@borod108 borod108 closed this Nov 15, 2018
@borod108 borod108 reopened this Nov 15, 2018
@borod108 borod108 force-pushed the rfe/ansible_cluster_upgrade branch 2 times, most recently from 5b10155 to 5f684e9 Compare November 19, 2018 15:52
@ems = FactoryGirl.create(:ems_redhat_with_authentication)
@cluster = FactoryGirl.create(:ems_cluster_redhat, :ems_id => @ems.id)
my_server = double("my_server", :guid => "guid1")
allow(MiqServer).to receive(:my_server).and_return(my_server)
Copy link
Contributor

Choose a reason for hiding this comment

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

What receive(:my_server) stands for ? it doesn't seems like a method name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add method to upgrade cluster using ovirt-ansible-cluster-upgrade role.
@borod108 borod108 force-pushed the rfe/ansible_cluster_upgrade branch from 5f684e9 to 9df4a8a Compare November 19, 2018 16:50
@masayag
Copy link
Contributor

masayag commented Nov 19, 2018

@borod108 there's a typo in header Ansibel --> Ansible

@borod108 borod108 force-pushed the rfe/ansible_cluster_upgrade branch from e21f22b to e9ea47c Compare November 20, 2018 05:26
@miq-bot
Copy link
Member

miq-bot commented Nov 20, 2018

Checked commits borod108/manageiq-providers-ovirt@9df4a8a~...e9ea47c with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 0 offenses detected
Everything looks fine. 👍

@borod108 borod108 added the wip label Nov 20, 2018
@borod108 borod108 changed the title Upgrade Ovirt Cluster through Ansibel [wip]Upgrade Ovirt Cluster through Ansibel Nov 20, 2018
@borod108
Copy link
Contributor Author

@agrare @Ladas so I manged to make it work, without moving the ems_cluster code to the core repo. So everything seems to work fine, but because we do not really have sit for ems cluster I would like you to review and tell me if it is ok to leave it like this. (The solution was setting the class_model to ::EmsCluster)

@Ladas
Copy link
Contributor

Ladas commented Nov 20, 2018

@borod108 hm, and does it store the right :type into EmsCluster record? I'd say it will not store the ManageIQ::Providers::Redhat::InfraManager::EmsCluster?

@@ -0,0 +1,29 @@
class ManageIQ::Providers::Redhat::InfraManager::EmsCluster < ::EmsCluster
Copy link
Contributor

@Ladas Ladas Nov 20, 2018

Choose a reason for hiding this comment

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

@borod108 lets try to define this as

module ManageIQ
  module Providers
    module Redhat
      class InfraManager
         class EmsCluster < ::EmsCluster
         end
      end
    end
  end
end

@@ -24,7 +24,9 @@ def initialize_infra_inventory_collections
# --- IC groups definitions ---

def add_clusters_group
add_collection(infra, :ems_clusters)
add_collection(infra, :ems_clusters) do |builder|
builder.add_properties(:model_class => ::EmsCluster)
Copy link
Contributor

Choose a reason for hiding this comment

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

@borod108 and with the nested definition ^, lets try

:model_class => ManageIQ::Providers::Redhat::InfraManager::EmsCluster

and then verify it stores the right :type into the DB

@borod108
Copy link
Contributor Author

Have to close this PR and move it to core, since EmsCluster does not have sti.

@borod108 borod108 closed this Nov 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants