You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@libcloud.apache.org by GitBox <gi...@apache.org> on 2022/01/14 22:55:32 UTC

[GitHub] [libcloud] rgharris opened a new pull request #1645: Create Azure node from Compute Gallery image, OS disk delete

rgharris opened a new pull request #1645:
URL: https://github.com/apache/libcloud/pull/1645


   
   
   ## Create Azure node from Compute Gallery image, OS disk delete
   
   ### Description
   
   Source issues:
   - #1643
   - #1644
   
   [Azure Compute Gallery](https://docs.microsoft.com/en-us/azure/virtual-machines/shared-image-galleries) is the recommended way to create custom images for Azure VMs. Supporting these works just like the `AzureImage` in libcloud today except you provide a single `id` under `imageReference` (instead of multiple key/values). See this [example request](https://docs.microsoft.com/en-us/rest/api/compute/virtual-machines/create-or-update#create-a-vm-from-a-custom-image) and the [corresponding spec](https://docs.microsoft.com/en-us/rest/api/compute/virtual-machines/create-or-update#imagereference). 
   
   Azure VM OS disks can be configured at the time of VM creation to be deleted when the VM is deleted. It would be helpful if libcloud supported this option. There is not an option in libcloud to do this on delete and it is convenient in some use cases to delete the disk regardless of the method (CLI, portal, libcloud, etc.). Supporting this requires a `deleteOption` of `Delete` under `OSDisk` when creating a VM. See the [OSDisk spec](https://docs.microsoft.com/en-us/rest/api/compute/virtual-machines/create-or-update#osdisk) for details. 
   
   A newer `api-version` is required for `create_node` (`deleteOption` not accepted otherwise).
   
   ### Status
   
   Done, ready for review. 
   
   The need to change the `api-version` for the Azure create node API may be problematic. I have tested `deleteOption` with Azure VMs using Compute Gallery Images with user code. I have added simple tests as well.
   
   ### Checklist (tick everything that applies)
   
   - [x] [Code linting](http://libcloud.readthedocs.org/en/latest/development.html#code-style-guide) (required, can be done after the PR checks)
   - [x] Documentation
   - [x] [Tests](http://libcloud.readthedocs.org/en/latest/testing.html)
   - [ ] [ICLA](http://libcloud.readthedocs.org/en/latest/development.html#contributing-bigger-changes) (required for bigger changes)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] codecov-commenter commented on pull request #1645: Create Azure node from Compute Gallery image, OS disk delete

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1645:
URL: https://github.com/apache/libcloud/pull/1645#issuecomment-1058507515


   # [Codecov](https://codecov.io/gh/apache/libcloud/pull/1645?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#1645](https://codecov.io/gh/apache/libcloud/pull/1645?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c3b12ec) into [trunk](https://codecov.io/gh/apache/libcloud/commit/90971e17bfd7b6bb97b2489986472c531cc8e140?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (90971e1) will **increase** coverage by `0.02%`.
   > The diff coverage is `83.33%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/libcloud/pull/1645/graphs/tree.svg?width=650&height=150&src=pr&token=PYoduksh69&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/libcloud/pull/1645?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##            trunk    #1645      +/-   ##
   ==========================================
   + Coverage   83.17%   83.20%   +0.02%     
   ==========================================
     Files         398      400       +2     
     Lines       86709    86926     +217     
     Branches     9217     9230      +13     
   ==========================================
   + Hits        72120    72325     +205     
   - Misses      11453    11462       +9     
   - Partials     3136     3139       +3     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/libcloud/pull/1645?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [libcloud/compute/drivers/azure\_arm.py](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGliY2xvdWQvY29tcHV0ZS9kcml2ZXJzL2F6dXJlX2FybS5weQ==) | `57.75% <69.23%> (+1.62%)` | :arrow_up: |
   | [libcloud/test/compute/test\_azure\_arm.py](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGliY2xvdWQvdGVzdC9jb21wdXRlL3Rlc3RfYXp1cmVfYXJtLnB5) | `98.80% <100.00%> (+0.13%)` | :arrow_up: |
   | [libcloud/test/compute/test\_openstack.py](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGliY2xvdWQvdGVzdC9jb21wdXRlL3Rlc3Rfb3BlbnN0YWNrLnB5) | `94.52% <0.00%> (-0.02%)` | :arrow_down: |
   | [libcloud/utils/py3.py](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGliY2xvdWQvdXRpbHMvcHkzLnB5) | `42.45% <0.00%> (ø)` | |
   | [libcloud/storage/types.py](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGliY2xvdWQvc3RvcmFnZS90eXBlcy5weQ==) | `100.00% <0.00%> (ø)` | |
   | [libcloud/storage/providers.py](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGliY2xvdWQvc3RvcmFnZS9wcm92aWRlcnMucHk=) | `57.14% <0.00%> (ø)` | |
   | [libcloud/compute/drivers/ec2.py](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGliY2xvdWQvY29tcHV0ZS9kcml2ZXJzL2VjMi5weQ==) | `75.49% <0.00%> (ø)` | |
   | [libcloud/compute/drivers/azure.py](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGliY2xvdWQvY29tcHV0ZS9kcml2ZXJzL2F6dXJlLnB5) | `65.37% <0.00%> (ø)` | |
   | [libcloud/test/common/test\_openstack.py](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGliY2xvdWQvdGVzdC9jb21tb24vdGVzdF9vcGVuc3RhY2sucHk=) | `100.00% <0.00%> (ø)` | |
   | [libcloud/test/storage/test\_scaleway.py](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGliY2xvdWQvdGVzdC9zdG9yYWdlL3Rlc3Rfc2NhbGV3YXkucHk=) | `100.00% <0.00%> (ø)` | |
   | ... and [5 more](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/libcloud/pull/1645?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/libcloud/pull/1645?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [90971e1...c3b12ec](https://codecov.io/gh/apache/libcloud/pull/1645?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] Kami commented on a change in pull request #1645: Create Azure node from Compute Gallery image, OS disk delete

Posted by GitBox <gi...@apache.org>.
Kami commented on a change in pull request #1645:
URL: https://github.com/apache/libcloud/pull/1645#discussion_r794911920



##########
File path: libcloud/compute/drivers/azure_arm.py
##########
@@ -703,7 +739,7 @@ def create_node(
 
         r = self.connection.request(
             target,
-            params={"api-version": RESOURCE_API_VERSION},

Review comment:
       As you pointed out in the PR description, changing this version may be problematic.
   
   To be on the safe side, I think it would be good to do the following:
   
   1. Add new ``api_version`` to the driver constructor and allow user to override default API version (we already use similar pattern in many other drives).
   2. Document this change in the upgrade_nodes.rst and show an example how user can utilize ``api_version`` constructor argument to set it to the previous default value.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] Kami commented on a change in pull request #1645: Create Azure node from Compute Gallery image, OS disk delete

Posted by GitBox <gi...@apache.org>.
Kami commented on a change in pull request #1645:
URL: https://github.com/apache/libcloud/pull/1645#discussion_r797993684



##########
File path: libcloud/compute/drivers/azure_arm.py
##########
@@ -703,7 +739,7 @@ def create_node(
 
         r = self.connection.request(
             target,
-            params={"api-version": RESOURCE_API_VERSION},

Review comment:
       We could start by trying with updating all the places to use the latest version and what's specified via ``api_version`` argument and then go from there - depending on if end to end tests pass or not. So in short, what you proposed via 2).
   
   Of course end to end tests do not cover every single possible scenario and setup, but it should be a start and give us at least some basic confidence.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] codecov-commenter edited a comment on pull request #1645: Create Azure node from Compute Gallery image, OS disk delete

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1645:
URL: https://github.com/apache/libcloud/pull/1645#issuecomment-1058507515


   # [Codecov](https://codecov.io/gh/apache/libcloud/pull/1645?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#1645](https://codecov.io/gh/apache/libcloud/pull/1645?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8c19357) into [trunk](https://codecov.io/gh/apache/libcloud/commit/90971e17bfd7b6bb97b2489986472c531cc8e140?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (90971e1) will **increase** coverage by `0.03%`.
   > The diff coverage is `86.84%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/libcloud/pull/1645/graphs/tree.svg?width=650&height=150&src=pr&token=PYoduksh69&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/libcloud/pull/1645?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##            trunk    #1645      +/-   ##
   ==========================================
   + Coverage   83.17%   83.20%   +0.03%     
   ==========================================
     Files         398      400       +2     
     Lines       86709    86953     +244     
     Branches     9217     9234      +17     
   ==========================================
   + Hits        72120    72350     +230     
   - Misses      11453    11463      +10     
   - Partials     3136     3140       +4     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/libcloud/pull/1645?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [libcloud/compute/constants/ec2\_instance\_types.py](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGliY2xvdWQvY29tcHV0ZS9jb25zdGFudHMvZWMyX2luc3RhbmNlX3R5cGVzLnB5) | `100.00% <ø> (ø)` | |
   | [...d/compute/constants/ec2\_region\_details\_complete.py](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGliY2xvdWQvY29tcHV0ZS9jb25zdGFudHMvZWMyX3JlZ2lvbl9kZXRhaWxzX2NvbXBsZXRlLnB5) | `100.00% <ø> (ø)` | |
   | [...ud/compute/constants/ec2\_region\_details\_partial.py](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGliY2xvdWQvY29tcHV0ZS9jb25zdGFudHMvZWMyX3JlZ2lvbl9kZXRhaWxzX3BhcnRpYWwucHk=) | `100.00% <ø> (ø)` | |
   | [libcloud/compute/drivers/azure.py](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGliY2xvdWQvY29tcHV0ZS9kcml2ZXJzL2F6dXJlLnB5) | `65.37% <ø> (ø)` | |
   | [libcloud/compute/drivers/ec2.py](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGliY2xvdWQvY29tcHV0ZS9kcml2ZXJzL2VjMi5weQ==) | `75.49% <ø> (ø)` | |
   | [libcloud/container/drivers/lxd.py](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGliY2xvdWQvY29udGFpbmVyL2RyaXZlcnMvbHhkLnB5) | `44.24% <0.00%> (ø)` | |
   | [libcloud/storage/providers.py](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGliY2xvdWQvc3RvcmFnZS9wcm92aWRlcnMucHk=) | `57.14% <ø> (ø)` | |
   | [libcloud/utils/py3.py](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGliY2xvdWQvdXRpbHMvcHkzLnB5) | `42.45% <ø> (ø)` | |
   | [libcloud/compute/drivers/vsphere.py](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGliY2xvdWQvY29tcHV0ZS9kcml2ZXJzL3ZzcGhlcmUucHk=) | `18.17% <20.00%> (ø)` | |
   | [libcloud/compute/drivers/azure\_arm.py](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGliY2xvdWQvY29tcHV0ZS9kcml2ZXJzL2F6dXJlX2FybS5weQ==) | `57.75% <69.81%> (+1.62%)` | :arrow_up: |
   | ... and [15 more](https://codecov.io/gh/apache/libcloud/pull/1645/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/libcloud/pull/1645?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/libcloud/pull/1645?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [90971e1...8c19357](https://codecov.io/gh/apache/libcloud/pull/1645?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] rgharris commented on a change in pull request #1645: Create Azure node from Compute Gallery image, OS disk delete

Posted by GitBox <gi...@apache.org>.
rgharris commented on a change in pull request #1645:
URL: https://github.com/apache/libcloud/pull/1645#discussion_r797146724



##########
File path: libcloud/compute/drivers/azure_arm.py
##########
@@ -703,7 +739,7 @@ def create_node(
 
         r = self.connection.request(
             target,
-            params={"api-version": RESOURCE_API_VERSION},

Review comment:
       Thanks for the feedback @Kami. 
   
   There are various hardcoded `api-version` values (e.g., "2019-06-01") in the driver in addition to many references to `RESOURCE_API_VERSION`.
   
   Which solution would fit best with the existing patterns for changes like this?
   
   1. Leave the existing hardcoded version values. Update all using `RESOURCE_API_VERSION` to use the updated value via `self.api_version` (allow override).
   2. Update all using `RESOURCE_API_VERSION` or hardcoded version values to use the updated value via `self.api_version` (allow override).
   3. Move just `create_node` to use the new API version and add an extra parameter allowing the version to be overridden. This doesn't align with your suggestion of course but does minimize the potential impact to other methods.
   
   1 & 2 do cause some potential issues: we will be affecting many methods, not just `create_node` that I have modified in this pull request. Updating the version for many methods is likely helpful as the current default is an older preview version. But further testing would be needed.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] Kami commented on a change in pull request #1645: Create Azure node from Compute Gallery image, OS disk delete

Posted by GitBox <gi...@apache.org>.
Kami commented on a change in pull request #1645:
URL: https://github.com/apache/libcloud/pull/1645#discussion_r798407407



##########
File path: libcloud/compute/drivers/azure_arm.py
##########
@@ -703,7 +739,7 @@ def create_node(
 
         r = self.connection.request(
             target,
-            params={"api-version": RESOURCE_API_VERSION},

Review comment:
       Thanks for checking.
   
   In this case, it sounds like it may be the best to only update api version used with the ``create_node()`` method.
   
   And to make code a bit easier to maintain going forward (and in the case user needs to override the API version for some reason), we should probably define those versions as part of module level constants - something along the lines of ``VMS_API_VERSION, DISKS_API_VERSION``, etc.
   
   If you want to try updating versions for other resources as well, that's fine, but it will likely require more work testing everything.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] rgharris commented on a change in pull request #1645: Create Azure node from Compute Gallery image, OS disk delete

Posted by GitBox <gi...@apache.org>.
rgharris commented on a change in pull request #1645:
URL: https://github.com/apache/libcloud/pull/1645#discussion_r807289420



##########
File path: libcloud/compute/drivers/azure_arm.py
##########
@@ -703,7 +739,7 @@ def create_node(
 
         r = self.connection.request(
             target,
-            params={"api-version": RESOURCE_API_VERSION},

Review comment:
       In c3b12ecb20872ed5b0af9391d4dc539f017c4844 I have reverted the version number change for the volume methods due to `properties.accountType` in the request changing to `sku.name` in the newer version. Found this after further manual testing. I've added a comment noting that ideally down the road all volume operations would be updated to use the same version. 
   
   @Kami do you think this PR is ready to be merged? If not, any suggestions on this API version consolidation?
   
   Updated list of version changes:
   ```
   # updated to latest version due to changes in create_node
   list_nodes 
   create_node 
   reboot_node
   destroy_node 
   ex_get_node
   start_node
   stop_node 
   
   # updated to version used by ex_delete_public_ip
   ex_get_public_ip 
   ex_list_public_ips
   ex_create_public_ip  
   
   # updated to version used by ex_update_nic_properties
   ex_create_network_interface
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] Kami merged pull request #1645: Create Azure node from Compute Gallery image, OS disk delete

Posted by GitBox <gi...@apache.org>.
Kami merged pull request #1645:
URL: https://github.com/apache/libcloud/pull/1645


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] rgharris commented on a change in pull request #1645: Create Azure node from Compute Gallery image, OS disk delete

Posted by GitBox <gi...@apache.org>.
rgharris commented on a change in pull request #1645:
URL: https://github.com/apache/libcloud/pull/1645#discussion_r798914355



##########
File path: libcloud/compute/drivers/azure_arm.py
##########
@@ -703,7 +739,7 @@ def create_node(
 
         r = self.connection.request(
             target,
-            params={"api-version": RESOURCE_API_VERSION},

Review comment:
       In 83e6f51d03ad9010093f44b29491a84cabb09858 I added constants per resource type API version. This required a few methods to use updated API versions due to inconsistencies within some resources. I have done some minimal testing of all methods. I would be okay with using this pattern or just adjusting `create_node()` to simplify testing, let me know what you think.
   
   These had version changes in the commit:
   ```
   # updated to latest version due to changes in create_node
   list_nodes 
   create_node 
   reboot_node
   destroy_node 
   ex_get_node
   start_node
   stop_node  
   
   # updated to version used by ex_resize_volume
   create_volume 
   list_volumes
   ex_get_volume 
   
   # updated to version used by ex_delete_public_ip
   ex_get_public_ip 
   ex_list_public_ips
   ex_create_public_ip  
   
   # updated to version used by ex_update_nic_properties
   ex_create_network_interface
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] rgharris commented on a change in pull request #1645: Create Azure node from Compute Gallery image, OS disk delete

Posted by GitBox <gi...@apache.org>.
rgharris commented on a change in pull request #1645:
URL: https://github.com/apache/libcloud/pull/1645#discussion_r798078978



##########
File path: libcloud/compute/drivers/azure_arm.py
##########
@@ -703,7 +739,7 @@ def create_node(
 
         r = self.connection.request(
             target,
-            params={"api-version": RESOURCE_API_VERSION},

Review comment:
       I gave that a try. It looks like Azure has different supported versions per service. For example, `2021-07-01` is supported for [VMs](https://docs.microsoft.com/en-us/rest/api/compute/virtual-machines) but not [disks](https://docs.microsoft.com/en-us/rest/api/compute/disks). [Public IP addresses](https://docs.microsoft.com/en-us/rest/api/virtualnetwork/public-ip-addresses) have differing versions as well. 
   
   So, using a common API version across all requests will not work. This does explain why newer code has used hardcoded version numbers. Some variation of solution (3) will likely be necessary. Or we could do a deeper dive into differences in the API versions and move to the new version with no override (for `create_node` and optionally other VM APIs) if no breaking changes. What are your thoughts on those options? 
   
   An error when using `2021-07-01` for `ex_get_volume`:
   > [NoRegisteredProviderFound] No registered resource provider found for location 'westus2' and API version '2021-07-01' for type 'disks'. The supported api-versions are '2016-04-30-preview, 2017-03-30, 2018-04-01, 2018-06-01, 2018-09-30, 2019-03-01, 2019-07-01, 2019-11-01, 2020-05-01, 2020-06-30, 2020-09-30, 2020-12-01, 2021-04-01, 2021-08-01, 2021-12-01'. The supported locations are 'southeastasia, eastus2, centralus, westeurope, eastus, northcentralus, southcentralus, westus, northeurope, eastasia, brazilsouth, westus2, westcentralus, ukwest, uksouth, japaneast, japanwest, canadacentral, canadaeast, centralindia, southindia, australiaeast, australiasoutheast, koreacentral, westindia, francecentral, southafricanorth, uaenorth, australiacentral, switzerlandnorth, germanywestcentral, norwayeast, jioindiawest, westus3, swedencentral'.
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [libcloud] Kami commented on pull request #1645: Create Azure node from Compute Gallery image, OS disk delete

Posted by GitBox <gi...@apache.org>.
Kami commented on pull request #1645:
URL: https://github.com/apache/libcloud/pull/1645#issuecomment-1024671878


   Thanks for the contribution.
   
   I added a comment on how we could make the API version change a bit safer in case that change brakes the behavior for some user / setup, besides that, LGTM.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org