-
Notifications
You must be signed in to change notification settings - Fork 12
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
Replace azurerm vm with azurerm_linux #176
Conversation
706582e
to
b60f288
Compare
b60f288
to
244c515
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.
No changes to propose but some questions and one more VR to run
} | ||
} | ||
|
||
source_image_id = var.sles4sap_uri != "" ? join(",", azurerm_image.sles4sap.*.id) : null |
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.
Why this rework was needed?
dynamic "source_image_reference" {
for_each = var.sles4sap_uri != "" ? [] : [1]
content {
publisher = module.os_image_reference.publisher
offer = module.os_image_reference.offer
sku = module.os_image_reference.sku
version = module.os_image_reference.version
}
}
source_image_id = var.sles4sap_uri != "" ? join(",", azurerm_image.sles4sap.*.id) : null
This deserve a dedicated VR
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.
Because source_image_id
was no longer supported inside the dynamic "source_image_reference"
, which replaces storage_image_reference
. Now there's source_image_reference
for catalogue images, and source_image_id
for custome images, but documentation states
NOTE:
One of either source_image_id or source_image_reference must be set.
I resorted to that code that checks for the existance of var.sles4sap_uri
and uses one or the other accordingly. It's basically the same logic as the existing code, just with the id separated as a different var outside the reference as the new specification states
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.
Thanks for the explanation Bill. I guess that means we need a VR for each method the image can be supplied, either via an URI which would mean we are testing an unpublished image from the Build Service, or by submitting a publisher, version, sku, etc, meaning we are testing on a catalog image.
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.
LGTM
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.
LGTM, but I think we need to add more verification runs to cover more the changes.
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.
LGTM
@alvarocarvajald suggest away! I'm running another one based on https://openqa.suse.de/tests/12289218, feel free to ask for others as well |
Off the top of my head, I'd say some from https://openqa.suse.de/group_overview/285 (HanaSR on PAYG and BYOS, saptune on PAYG & BYOS) which deal with unreleased OS images, and then perhaps some from https://openqaworker15.qa.suse.cz/group_overview/41 (I'd say 15-SP5 and 12-SP5, since you already have a passing one with 15-SP3). Finally some saptune ones from https://openqaworker15.qa.suse.cz/group_overview/46 (saptune on 15-SP5 PAYG mostly, since the previous 15-SP5 ones are BYOS) and the same from https://openqaworker15.qa.suse.cz/group_overview/28 (saptune on 12-SP5 ... but last MU which triggered tests in this job group is 18 days old, so leave this last one as optional) |
5af27d8
to
f775ec9
Compare
f775ec9
to
e7712cb
Compare
e7712cb
to
e4c14be
Compare
@BillAnastasiadis Some VRs failed. Do you know why? (The operation was succesful. The patient died.) |
@a-kpappas someone edited the description and answered :D |
LGTM |
This pr replaces all cases of
azurerm_virtual_machine
, which is deprecated, withazurerm_linux_virtual_machine
. The conversion demanded a LOT of other changes as well, as the 2 resources have a lot of differences in the way they define and use their components.Some of the differences (they are too many to list all of them) are:
azurerm_virtual_machine
resource, now need to be defined as a separate resource together with an attachment resource that binds them to the respective VMs.vm_size
renamed tosize
os_profile
andos_profile_linux_config
blocks are replaced with top-level arguments inazurerm_linux_virtual_machine
delete_os_disk_on_termination
anddelete_data_disks_on_termination
attributes are removed from theazurerm_linux_virtual_machine
resource. The first is now moved to provider level and renamed (provider->features->delete_os_disk_on_deletion
), while the second has disappeared completely (!!) and is no longer doable in terraform. Ricardo said thet pcw takes care of that cleanupboot_diagnostics
changes:enabled
attribute no longer supported, whilestorage_uri
is renamed tostorage_account_uri
LINKS:
HanaSr-Azure-(BYOS|PAYG):
15sp3 BYOS:
https://openqaworker15.qa.suse.cz/tests/236441 🟢
Qesap-Azure-(BYOS|PAYG) (failure after deployment expected):
15sp5:
https://openqaworker15.qa.suse.cz/tests/236437 red is later after the deployment and quite like due to TEAM-8442
15sp4:
https://openqaworker15.qa.suse.cz/tests/236439 red is later after the deployment and quite like due to TEAM-8442
12sp5:
https://openqaworker15.qa.suse.cz/tests/237155 🟢
http://openqaworker15.qa.suse.cz/tests/237156 red is later after the deployment and quite like due to TEAM-8442
qesap-azure-saptune-uri:
https://openqa.suse.de/tests/12325535 red is later after the deployment and quite like due to TEAM-8442