-
Notifications
You must be signed in to change notification settings - Fork 24
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
Making changes for ForemanAzureRM with azure-sdk-for-ruby #36
Conversation
18d66a4
to
fb0a65b
Compare
As an update, the PR now completes the orchestration steps successfully with both User data and Finish template approach along with password and ssh-key based authentication mechanisms. However, the PR is still in WIP state as the code still needs refining. |
ee7c325
to
a407146
Compare
5861fa1
to
c3372fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need to make sure the tests are running correctly. Right now Travis fails to load the tests:
/home/travis/.rvm/gems/ruby-2.3.8/gems/factory_bot-5.0.2/lib/factory_bot/definition_proxy.rb:97:in `method_missing': undefined method 'provider' in 'azure_cr' factory (NoMethodError)
from /home/travis/build/theforeman/foreman_azure_rm/test/factories/azure.rb:3:in `block (2 levels) in <top (required)>'
from /home/travis/.rvm/gems/ruby-2.3.8/gems/factory_bot-5.0.2/lib/factory_bot/syntax/default.rb:18:in `instance_eval'
from /home/travis/.rvm/gems/ruby-2.3.8/gems/factory_bot-5.0.2/lib/factory_bot/syntax/default.rb:18:in `factory'
from /home/travis/build/theforeman/foreman_azure_rm/test/factories/azure.rb:2:in `block in <top (required)>'
from /home/travis/.rvm/gems/ruby-2.3.8/gems/factory_bot-5.0.2/lib/factory_bot/syntax/default.rb:49:in `instance_eval'
from /home/travis/.rvm/gems/ruby-2.3.8/gems/factory_bot-5.0.2/lib/factory_bot/syntax/default.rb:49:in `run'
from /home/travis/.rvm/gems/ruby-2.3.8/gems/factory_bot-5.0.2/lib/factory_bot/syntax/default.rb:7:in `define'
from /home/travis/build/theforeman/foreman_azure_rm/test/factories/azure.rb:1:in `<top (required)>'
from /home/travis/.rvm/gems/ruby-2.3.8/gems/activesupport-5.2.1/lib/active_support/dependencies.rb:281:in `load'
from /home/travis/.rvm/gems/ruby-2.3.8/gems/activesupport-5.2.1/lib/active_support/dependencies.rb:281:in `block in load'
from /home/travis/.rvm/gems/ruby-2.3.8/gems/activesupport-5.2.1/lib/active_support/dependencies.rb:253:in `load_dependency'
from /home/travis/.rvm/gems/ruby-2.3.8/gems/activesupport-5.2.1/lib/active_support/dependencies.rb:281:in `load'
from /home/travis/.rvm/gems/ruby-2.3.8/gems/factory_bot-5.0.2/lib/factory_bot/find_definitions.rb:20:in `block (2 levels) in find_definitions'
from /home/travis/.rvm/gems/ruby-2.3.8/gems/factory_bot-5.0.2/lib/factory_bot/find_definitions.rb:19:in `each'
from /home/travis/.rvm/gems/ruby-2.3.8/gems/factory_bot-5.0.2/lib/factory_bot/find_definitions.rb:19:in `block in find_definitions'
from /home/travis/.rvm/gems/ruby-2.3.8/gems/factory_bot-5.0.2/lib/factory_bot/find_definitions.rb:15:in `each'
from /home/travis/.rvm/gems/ruby-2.3.8/gems/factory_bot-5.0.2/lib/factory_bot/find_definitions.rb:15:in `find_definitions'
from /home/travis/.rvm/gems/ruby-2.3.8/gems/factory_bot-5.0.2/lib/factory_bot/reload.rb:6:in `reload'
from /home/travis/build/theforeman/foreman_azure_rm/test/test_plugin_helper.rb:6:in `<top (required)>'
from /home/travis/build/theforeman/foreman_azure_rm/test/functional/azure_rm_test.rb:1:in `require_relative'
from /home/travis/build/theforeman/foreman_azure_rm/test/functional/azure_rm_test.rb:1:in `<top (required)>'
from /home/travis/.rvm/gems/ruby-2.3.8/gems/rake-12.3.2/lib/rake/rake_test_loader.rb:17:in `require'
from /home/travis/.rvm/gems/ruby-2.3.8/gems/rake-12.3.2/lib/rake/rake_test_loader.rb:17:in `block in <main>'
from /home/travis/.rvm/gems/ruby-2.3.8/gems/rake-12.3.2/lib/rake/rake_test_loader.rb:5:in `select'
from /home/travis/.rvm/gems/ruby-2.3.8/gems/rake-12.3.2/lib/rake/rake_test_loader.rb:5:in `<main>'
end | ||
|
||
def vm_size=(setvsize) | ||
@azure_vm.hardware_profile.vm_size = setvsize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably need to initialize hardware_profile
in case it was not initialized before . Especially when setting up an empty instance.
@azure_vm.hardware_profile.vm_size = setvsize | ||
end | ||
|
||
def platform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the same comment for all properties here: we should implement all getters either by using delegate
or by implementing them manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we are defining only foreman specific methods in AzureRMCompute class, hence removing the other methods and added delegation for id
and name
methods.
end | ||
|
||
def os_disk_size=(setosdisk) | ||
@azure_vm.os_disk.disk_size_gb = setosdisk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to initialize os_disk
in case it is not initialized before.
# List all VMs in a resource group | ||
virtual_machines = compute_client.virtual_machines.list(rg_name) | ||
rg_vms = [] | ||
virtual_machines.each do |vm| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can return virtual_machines
directly.
@vijay8451 Are you selecting the Image first from |
@apuntamb it is like this: |
7545f94
to
fd13ec6
Compare
@vijay8451 Now, it should work as expected. I've pushed the changes. |
@apuntamb I am able to list the subnet and same is showing over |
test/functional/azure_rm_test.rb
Outdated
test "should create a compute resource and return edit page" do | ||
test_client = Fog::Resources::AzureRM.new( | ||
test_sdk = ForemanAzureRM::AzureSDKAdapter.new( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShimShtein Do you think this test_sdk object should be defined outside the test so that other tests would use it too? Can we do:
test_sdk = ForemanAzureRM::AzureSDKAdapter.new(
tenant_id: '',
client_id: '',
client_secret: '',
subscription_id: ''
)
test "should create a compute resource and return edit page" do
.......
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can make the sdk mock available in all tests through a class variable in setup
block:
class ComputeResourcesControllerTest
setup do
@test_sdk = mock('test_sdk') #creates an empty object
ForemanAzureRM::AzureRM.any_instance.stubs(:sdk).returns(test_sdk)
@test_resource_client = mock('resource_client')
test_sdk.stubs(:resource_client).returns(test_resource_client)
end
test 'do some tests'
@test_resource_client.stubs(:rgs).returns([])
# ...
end
test 'do other tests'
@test_resource_client.stubs(:rgs).returns(['a', 'b', 'c'])
# ...
end
end
response = sdk.create_or_update_vm(vm_hash[:resource_group], vm_hash[:name], vm_create_params) | ||
|
||
rescue MsRestAzure::AzureOperationError => e | ||
puts "**********FAILURE!!!***********" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should ignore Azure exceptions here
test/functional/azure_rm_test.rb
Outdated
@@ -4,19 +4,18 @@ module ForemanAzureRM | |||
class ComputeResourcesControllerTest < ActionController::TestCase | |||
tests ::ComputeResourcesController | |||
|
|||
setup { Fog.mock! } | |||
teardown { Fog.unmock! } | |||
|
|||
test "should create a compute resource and return edit page" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of the test is a bit misleading here, it does not create a compute resource. I displays valid edit form for existing compute resource.
test/functional/azure_rm_test.rb
Outdated
ForemanAzureRM::AzureRM.any_instance.stubs(:sdk).returns(test_sdk) | ||
test_resource_client = Object.new | ||
test_sdk.stubs(:resource_client).returns(test_resource_client) | ||
test_resource_client.stubs(:rgs).returns([]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a common case that resource groups would return an empty array? If it is, I would suggest adding another test with a non-empty array and validates that we are displaying the groups correctly. If this is an edge case - we can probably modify the current test to work with non-empty array.
test/functional/azure_rm_test.rb
Outdated
test "should create a compute resource and return edit page" do | ||
test_client = Fog::Resources::AzureRM.new( | ||
test_sdk = ForemanAzureRM::AzureSDKAdapter.new( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can make the sdk mock available in all tests through a class variable in setup
block:
class ComputeResourcesControllerTest
setup do
@test_sdk = mock('test_sdk') #creates an empty object
ForemanAzureRM::AzureRM.any_instance.stubs(:sdk).returns(test_sdk)
@test_resource_client = mock('resource_client')
test_sdk.stubs(:resource_client).returns(test_resource_client)
end
test 'do some tests'
@test_resource_client.stubs(:rgs).returns([])
# ...
end
test 'do other tests'
@test_resource_client.stubs(:rgs).returns(['a', 'b', 'c'])
# ...
end
end
@apuntamb Fails:
|
fd13ec6
to
4fb4ddf
Compare
@apuntamb Pass:
Fails/Issues:
|
e909759
to
cf8f40f
Compare
UPDATE: |
62eb81f
to
ae083ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apuntamb, we need to make Travis green again before we merge this PR.
Issue created: #37 to track one major refactor.
Probably will add a lot of smaller refactoring issues later.
@vijay8451 Could you please make a round of testing before I merge the PR? I don't see any major changes to the code anymore. There is only the issue with Travis, but it shouldn't make any difference as for testing.
@ShimShtein @apuntamb Thanks for the update. I'll start testing part on below but it would great to have update on: |
@ShimShtein thanks for the review. I'll start fixing Travis errors now, let's see why it goes red. Also, about the refactor issue you've created, I'll now take a look at it. |
@vijay8451 I'll start marking redmine issues (if any) that you should take a look while testing. |
@vijay8451 please take a look at the #27271 tracker that has issues linked which are ready for testing. |
@vijay8451 For your reference, this PR will cover the following use cases:
|
ae083ef
to
56758ed
Compare
QE Test Status: Pass:
New Issues/Failed tests:
Others:
Open Questions/Queries:
|
add_attribute(:user) { 'azurermuser' } | ||
add_attribute(:password) { 'azurermpassword' } | ||
add_attribute(:url) { 'http://azurerm.example.com' } | ||
add_attribute(:uuid) { 'azurermuuid' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be much helpful , if use the mock data format as using in actually:
i.e. add_attribute(:uuid) { 'a0a086e2-da7a-4059-aafe-9cc07cc519cd' }
same for user, password, url.
56758ed
to
5c4500d
Compare
@vijay8451 Thanks for bringing the issues into our notice. |
Below are working now:
Below is an open one only:
Note: Just to clear, other reported issues are required to fix as well but currently not a blocker for this PR. |
Thanks, @apuntamb and @vijay8451! |
@vijay8451, about the ssh keys issue: from what I see, if you set up admin user instead of root, it does put the certificate in both, but it disables ssh login by the
I think it's functionality that is provided by Azure. |
@ShimShtein thanks .. |
This PR is a WIP task. It deals with making changes to the foreman_azure_rm code base in alignment with the azure sdk for ruby. This PR is the first step towards using the azure-sdk instead of fog libraries.
This PR is expected to have couple of changes either with respect to functionalities or refactoring during it's review phase.
This PR shall allow user to:
Although a host and other resources such as OSdisk, NIC, PIP and VM will be created and seen on the Azure portal, yet, the orchestration steps from Foreman are still in progress to be successful.