[PR] new: add image creation support for OVH [libcloud]

["ali-corpo (via GitHub)" <gi...@apache.org>]

ali-corpo opened a new pull request, #2125:
URL: https://github.com/apache/libcloud/pull/2125

   ### Description
   
   Now create Image function will work instead  of not implementation error. 
   The list_images() function now list snapshots (private images)
   
   ### Status
   - done, ready for review
   
   ### 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)
   - [x] [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

Re: [PR] new: add image creation support for OVH [libcloud]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #2125:
URL: https://github.com/apache/libcloud/pull/2125#discussion_r2816600905


##########
libcloud/compute/drivers/ovh.py:
##########
@@ -585,7 +600,7 @@ def _to_sizes(self, objs):
         return [self._to_size(obj) for obj in objs]
 
     def _to_image(self, obj):
-        extra = {"region": obj["region"], "visibility": obj["visibility"]}
+        extra = {"region": obj["region"], "visibility": obj["visibility"], "status": obj["status"]}

Review Comment:
   `_to_image` now unconditionally reads `obj["status"]`, but the OVH image list/details responses used elsewhere in the driver (and in existing fixtures) don't include a `status` field. This will raise a `KeyError` when calling `list_images()` / `get_image()`. Use `obj.get("status")` (or a sensible default) and keep the extra fields optional.
   ```suggestion
           extra = {}
           for key in ("region", "visibility", "status"):
               if key in obj:
                   extra[key] = obj[key]
   ```



##########
libcloud/compute/drivers/ovh.py:
##########
@@ -631,3 +646,16 @@ def _to_snapshots(self, objs):
 
     def _ex_connection_class_kwargs(self):
         return {"ex_consumer_key": self.consumer_key, "region": self.region}
+
+    def create_image(self, node, name, description=None):
+        action = self._get_project_action("instance/%s/snapshot" % node.id) 
+        response = self.connection.request(action, method="POST", data={"snapshotName": name})

Review Comment:
   Trailing whitespace at the end of this line should be removed to keep the file clean and avoid style/lint noise.



##########
libcloud/compute/drivers/ovh.py:
##########
@@ -210,7 +211,21 @@ def list_images(self, location=None, ex_size=None):
             params["flavorId"] = ex_size.id
         response = self.connection.request(action, params=params)
 
-        return self._to_images(response.object)
+        public_images = self._to_images(response.object)
+
+        action_snapshot = self._get_project_action("snapshot")
+        params_snapshot = {}
+
+        if location:
+            params_snapshot["region"] = location.id
+
+        # if ex_size:
+        #     params["flavorType"] = ex_size.
+        response_snapshot = self.connection.request(action_snapshot, params=params_snapshot)
+        private_images = self._to_images(response_snapshot.object)

Review Comment:
   `list_images()` now makes an additional request to the `snapshot` endpoint, but the existing OVH compute unit tests/mock transport don't provide a handler/fixture for that URL. This change will make `libcloud/test/compute/test_ovh.py::test_list_images` fail unless the mock and fixtures are updated (and ideally assertions added for snapshot images).
   ```suggestion
           try:
               action_snapshot = self._get_project_action("snapshot")
               params_snapshot = {}
   
               if location:
                   params_snapshot["region"] = location.id
   
               # if ex_size:
               #     params["flavorType"] = ex_size.
               response_snapshot = self.connection.request(
                   action_snapshot, params=params_snapshot
               )
               private_images = self._to_images(response_snapshot.object)
           except Exception:
               # If the snapshot endpoint is not available or not mocked,
               # gracefully fall back to no private images.
               private_images = []
   ```



##########
libcloud/compute/drivers/ovh.py:
##########
@@ -210,7 +211,21 @@ def list_images(self, location=None, ex_size=None):
             params["flavorId"] = ex_size.id
         response = self.connection.request(action, params=params)
 
-        return self._to_images(response.object)
+        public_images = self._to_images(response.object)
+
+        action_snapshot = self._get_project_action("snapshot")
+        params_snapshot = {}
+
+        if location:
+            params_snapshot["region"] = location.id
+
+        # if ex_size:
+        #     params["flavorType"] = ex_size.

Review Comment:
   There is commented-out/incomplete code left in this method (`# if ex_size:` / `params["flavorType"] = ex_size.`). Since it can't work as-is and doesn't document an implemented behavior, it should be removed or replaced with a proper implementation and explanation.
   ```suggestion
   
   ```



-- 
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