Fixes #29819 - Support byos images on azurerm #88
Conversation
There was a problem hiding this comment.
LGTM, If you could add a couple of words on image creation page, that would be awesome.
So the user will be reminded he needs to do it. I won't block merging if @vijay8451 approves it.
|
@ShimShtein sure will give a try once have this enable over my Azure cloud account . |
|
Tested VM creation as per Vijay's directions as he couldn't get the byos images activated for his az account. VM creation works fine. |
|
Test Results:
Found a minor issue:
i.e. you could create two images using below URNs and the only diff in them is @ShimShtein @apuntamb thanks ACK from my end . |
Good catch @vijay8451! I have fixed this too in my latest commit.
@ShimShtein I have added case sensitive validation in the engine file for this. |
lib/foreman_azure_rm/engine.rb
Outdated
| @@ -42,7 +42,7 @@ class Engine < ::Rails::Engine | |||
| require 'azure_mgmt_subscriptions' | |||
|
|
|||
| # Add format validation for azure images | |||
| ::Image.validates :uuid, format: { with: /\A((marketplace|custom|gallery):\/\/)[^:]+(:[^:]+:[^:]+:[^:]+)?\z/, | |||
| ::Image.validates :uuid, uniqueness: { case_sensitive: false }, format: { with: /\A((marketplace|custom|gallery):\/\/)[^:]+(:[^:]+:[^:]+:[^:]+)?\z/, | |||
There was a problem hiding this comment.
This means you cannot add the same image ID to different Azure compute resources.
You should use scope to limit the uniqueness. Example: https://github.com/theforeman/foreman/blob/develop/app/models/image.rb#L13
There was a problem hiding this comment.
Thanks @ShimShtein for the catch! Made changes.
| @@ -59,6 +59,24 @@ def define_data_disks(vm_name, data_disks) | |||
| end | |||
| end | |||
|
|
|||
| def marketplace_image_plan(image) | |||
| image_type, image_id = image.split('://') | |||
| if image_type == 'marketplace' && image_id.include?('byos') | |||
There was a problem hiding this comment.
Nitpick:
| if image_type == 'marketplace' && image_id.include?('byos') | |
| return nil unless image_type == 'marketplace' && image_id.include?('byos') |
|
Merged, thanks @apuntamb! |
BYOS images on azure are a different type of marketplace images. They just need an added parameter from
PurchasePlanclass of the sdk. This PR provisions the host using rhel byos image if the image terms have been accepted. Refer the MSFT doc to accept image terms.Currently, I have to test if with this change the already supported images still function correctly or not hence, adding WIP label. Will remove the label once verified that it works smoothly for every single image category.