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 2019/12/26 04:30:08 UTC

[GitHub] [libcloud] c-w opened a new pull request #1397: Enable `ex_prefix` to be passed via all storage SDK methods

c-w opened a new pull request #1397: Enable `ex_prefix` to be passed via all storage SDK methods
URL: https://github.com/apache/libcloud/pull/1397
 
 
   ## Enable `ex_prefix` to be passed via all storage SDK methods
   
   ### Description
   
   There currently is an inconsistency in the storage SDK methods to list/iterate container objects since according to method signatures in the storage base classes `ex_prefix` can only be provided via `driver.list_container_objects` but not via `driver.iterate_container_objects`, `container.list_objects`, or `container.iterate_objects`.
   
   This pull request fixes this inconsistency by adding `ex_prefix` to all the method signatures and also implements `ex_prefix` for the local storage provider.
   
   ### Status
   
   - done, ready for review
   
   ### Checklist (tick everything that applies)
   
   - [x] Code linting ([build passed](https://travis-ci.org/CatalystCode/libcloud/builds/629544380))
   - [x] Documentation (updated existing docstrings)
   - [x] Tests (updated existing tests)
   - [x] ICLA ([committer](http://people.apache.org/phonebook.html?uid=clewolff))
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [libcloud] c-w commented on issue #1397: Enable `ex_prefix` to be passed via all storage SDK methods

Posted by GitBox <gi...@apache.org>.
c-w commented on issue #1397: Enable `ex_prefix` to be passed via all storage SDK methods
URL: https://github.com/apache/libcloud/pull/1397#issuecomment-569526239
 
 
   Thanks for the additional context @Kami. I'll work on the change and update this PR.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [libcloud] c-w commented on issue #1397: Enable `ex_prefix` to be passed via all storage SDK methods

Posted by GitBox <gi...@apache.org>.
c-w commented on issue #1397: Enable `ex_prefix` to be passed via all storage SDK methods
URL: https://github.com/apache/libcloud/pull/1397#issuecomment-569384089
 
 
   @Kami Would promoting `ex_prefix` to `prefix` be a breaking API change or would we also keep the `ex_prefix` argument? If we're considering a breaking API change, I'd suggest to first merge this pull request (which is backwards compatible so potentially releasable via a patch version) and consider the argument promotion for a future change.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [libcloud] Kami commented on a change in pull request #1397: Promote `ex_prefix` to top-level storage SDK argument

Posted by GitBox <gi...@apache.org>.
Kami commented on a change in pull request #1397: Promote `ex_prefix` to top-level storage SDK argument
URL: https://github.com/apache/libcloud/pull/1397#discussion_r364075769
 
 

 ##########
 File path: libcloud/storage/drivers/atmos.py
 ##########
 @@ -451,11 +451,35 @@ def _emc_meta(self, response):
         meta = meta.split(', ')
         return dict([x.split('=', 1) for x in meta])
 
-    def iterate_container_objects(self, container):
+    def _entries_to_objects(self, container, entries):
+        for entry in entries:
+            metadata = {'object_id': entry['id']}
+            yield Object(entry['name'], 0, '', {}, metadata, container, self)
+
+    def iterate_container_objects(self, container, prefix=None,
+                                  ex_prefix=None):
+        """
+        Return a generator of objects for the given container.
+
+        :param container: Container instance
+        :type container: :class:`Container`
+
+        :param prefix: Filter objects starting with a prefix.
+                       Filtering is performed client-side.
+        :type  prefix: ``str``
+
+        :param ex_prefix: (Deprecated.) Filter objects starting with a prefix.
+                          Filtering is performed client-side.
+        :type  ex_prefix: ``str``
+
+        :return: A generator of Object instances.
+        :rtype: ``generator`` of :class:`Object`
+        """
+        prefix = self._normalize_prefix_argument(prefix, ex_prefix)
+
         headers = {'x-emc-include-meta': '1'}
         path = self._namespace_path(container.name) + '/'
         result = self.connection.request(path, headers=headers)
         entries = self._list_objects(result.object, object_type='regular')
-        for entry in entries:
-            metadata = {'object_id': entry['id']}
-            yield Object(entry['name'], 0, '', {}, metadata, container, self)
+        objects = self._entries_to_objects(container, entries)
+        return self._filter_listed_container_objects(objects, prefix)
 
 Review comment:
   I was thinking about doing this a bit differently, but that approach also works.
   
   We just need to make sure that all the drivers which don't implement custom / server side based filtering call that method.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [libcloud] c-w commented on issue #1397: Promote `ex_prefix` to top-level storage SDK argument

Posted by GitBox <gi...@apache.org>.
c-w commented on issue #1397: Promote `ex_prefix` to top-level storage SDK argument
URL: https://github.com/apache/libcloud/pull/1397#issuecomment-572335948
 
 
   Added upgrade note in 16c692f.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [libcloud] Kami commented on issue #1397: Enable `ex_prefix` to be passed via all storage SDK methods

Posted by GitBox <gi...@apache.org>.
Kami commented on issue #1397: Enable `ex_prefix` to be passed via all storage SDK methods
URL: https://github.com/apache/libcloud/pull/1397#issuecomment-569446022
 
 
   @c-w That's correct, but since it wasn't previously part of the base API, we probably should only update drivers and methods which supported ``ex_prefix`` argument before, to also support it for backward compatibility reasons going forward (at appears those are azure, cloudfiles, oss, dummy and s3 drivers).
   
   And inside those methods we should probably just do something like ``prefix = prefix or ex_prefix`` and document in upgrade notes that the new argument name is ``prefix`` which has precedence over ``ex_prefix``, which was left in place for backward compatibility reasons.
   
   In short - I would just like to avoid adding ``ex_prefix`` to the base API since base API shouldn't really have any ``ex_`` prefixed arguments and methods.
   
   If you won't get a chance to make those changes in the near future, I can also look into it.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [libcloud] codecov-io commented on issue #1397: Promote `ex_prefix` to top-level storage SDK argument

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #1397: Promote `ex_prefix` to top-level storage SDK argument
URL: https://github.com/apache/libcloud/pull/1397#issuecomment-569533634
 
 
   # [Codecov](https://codecov.io/gh/apache/libcloud/pull/1397?src=pr&el=h1) Report
   > Merging [#1397](https://codecov.io/gh/apache/libcloud/pull/1397?src=pr&el=desc) into [trunk](https://codecov.io/gh/apache/libcloud/commit/043bb6d46248384bb6bc4357aa47c0c201d16b7b?src=pr&el=desc) will **decrease** coverage by `<.01%`.
   > The diff coverage is `94.73%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/libcloud/pull/1397/graphs/tree.svg?width=650&token=PYoduksh69&height=150&src=pr)](https://codecov.io/gh/apache/libcloud/pull/1397?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##            trunk    #1397      +/-   ##
   ==========================================
   - Coverage   86.58%   86.57%   -0.01%     
   ==========================================
     Files         364      364              
     Lines       76193    76209      +16     
     Branches     7439     7441       +2     
   ==========================================
   + Hits        65968    65981      +13     
   - Misses       7400     7402       +2     
   - Partials     2825     2826       +1
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/libcloud/pull/1397?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [libcloud/test/storage/test\_s3.py](https://codecov.io/gh/apache/libcloud/pull/1397/diff?src=pr&el=tree#diff-bGliY2xvdWQvdGVzdC9zdG9yYWdlL3Rlc3RfczMucHk=) | `91.47% <ø> (ø)` | :arrow_up: |
   | [libcloud/test/storage/test\_azure\_blobs.py](https://codecov.io/gh/apache/libcloud/pull/1397/diff?src=pr&el=tree#diff-bGliY2xvdWQvdGVzdC9zdG9yYWdlL3Rlc3RfYXp1cmVfYmxvYnMucHk=) | `92% <ø> (ø)` | :arrow_up: |
   | [libcloud/test/storage/test\_cloudfiles.py](https://codecov.io/gh/apache/libcloud/pull/1397/diff?src=pr&el=tree#diff-bGliY2xvdWQvdGVzdC9zdG9yYWdlL3Rlc3RfY2xvdWRmaWxlcy5weQ==) | `89.38% <ø> (ø)` | :arrow_up: |
   | [libcloud/test/storage/test\_dummy.py](https://codecov.io/gh/apache/libcloud/pull/1397/diff?src=pr&el=tree#diff-bGliY2xvdWQvdGVzdC9zdG9yYWdlL3Rlc3RfZHVtbXkucHk=) | `90% <ø> (ø)` | :arrow_up: |
   | [libcloud/test/storage/test\_oss.py](https://codecov.io/gh/apache/libcloud/pull/1397/diff?src=pr&el=tree#diff-bGliY2xvdWQvdGVzdC9zdG9yYWdlL3Rlc3Rfb3NzLnB5) | `95.34% <ø> (ø)` | :arrow_up: |
   | [libcloud/test/storage/test\_local.py](https://codecov.io/gh/apache/libcloud/pull/1397/diff?src=pr&el=tree#diff-bGliY2xvdWQvdGVzdC9zdG9yYWdlL3Rlc3RfbG9jYWwucHk=) | `90.59% <100%> (+0.14%)` | :arrow_up: |
   | [libcloud/storage/drivers/cloudfiles.py](https://codecov.io/gh/apache/libcloud/pull/1397/diff?src=pr&el=tree#diff-bGliY2xvdWQvc3RvcmFnZS9kcml2ZXJzL2Nsb3VkZmlsZXMucHk=) | `79.63% <100%> (-0.05%)` | :arrow_down: |
   | [libcloud/storage/drivers/s3.py](https://codecov.io/gh/apache/libcloud/pull/1397/diff?src=pr&el=tree#diff-bGliY2xvdWQvc3RvcmFnZS9kcml2ZXJzL3MzLnB5) | `89.15% <100%> (-0.03%)` | :arrow_down: |
   | [libcloud/storage/drivers/oss.py](https://codecov.io/gh/apache/libcloud/pull/1397/diff?src=pr&el=tree#diff-bGliY2xvdWQvc3RvcmFnZS9kcml2ZXJzL29zcy5weQ==) | `70.55% <100%> (-0.08%)` | :arrow_down: |
   | [libcloud/storage/drivers/local.py](https://codecov.io/gh/apache/libcloud/pull/1397/diff?src=pr&el=tree#diff-bGliY2xvdWQvc3RvcmFnZS9kcml2ZXJzL2xvY2FsLnB5) | `73.26% <100%> (+0.26%)` | :arrow_up: |
   | ... and [5 more](https://codecov.io/gh/apache/libcloud/pull/1397/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/libcloud/pull/1397?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/libcloud/pull/1397?src=pr&el=footer). Last update [043bb6d...1ea7b36](https://codecov.io/gh/apache/libcloud/pull/1397?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [libcloud] c-w merged pull request #1397: Promote `ex_prefix` to top-level storage SDK argument

Posted by GitBox <gi...@apache.org>.
c-w merged pull request #1397: Promote `ex_prefix` to top-level storage SDK argument
URL: https://github.com/apache/libcloud/pull/1397
 
 
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [libcloud] Kami commented on issue #1397: Enable `ex_prefix` to be passed via all storage SDK methods

Posted by GitBox <gi...@apache.org>.
Kami commented on issue #1397: Enable `ex_prefix` to be passed via all storage SDK methods
URL: https://github.com/apache/libcloud/pull/1397#issuecomment-569141710
 
 
   Overall, I'm fine with this change, but another option would be to simply name the argument ``prefix`` (aka promote it to be part of the base API).
   
   Then we could also have a fallback, if driver doesn't implement ``_filter_objects_by_prefix`` method (or similar) we would simply fall back to the implementation on the base driver which performs simple local filtering on the whole result set.
   
   Since that method would perform filtering locally, it would probably also be a good idea to document that behavior (perhaps also emit a warning?) to avoid surprises.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [libcloud] codecov-io edited a comment on issue #1397: Promote `ex_prefix` to top-level storage SDK argument

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #1397: Promote `ex_prefix` to top-level storage SDK argument
URL: https://github.com/apache/libcloud/pull/1397#issuecomment-569533634
 
 
   # [Codecov](https://codecov.io/gh/apache/libcloud/pull/1397?src=pr&el=h1) Report
   > Merging [#1397](https://codecov.io/gh/apache/libcloud/pull/1397?src=pr&el=desc) into [trunk](https://codecov.io/gh/apache/libcloud/commit/1ea7b36185ef9974c2abb5ee332a3342fc0aa1d1?src=pr&el=desc) will **increase** coverage by `<.01%`.
   > The diff coverage is `100%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/libcloud/pull/1397/graphs/tree.svg?width=650&token=PYoduksh69&height=150&src=pr)](https://codecov.io/gh/apache/libcloud/pull/1397?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##            trunk    #1397      +/-   ##
   ==========================================
   + Coverage   86.57%   86.58%   +<.01%     
   ==========================================
     Files         364      364              
     Lines       76209    76224      +15     
     Branches     7441     7443       +2     
   ==========================================
   + Hits        65981    65996      +15     
     Misses       7402     7402              
     Partials     2826     2826
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/libcloud/pull/1397?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [libcloud/test/compute/test\_ec2.py](https://codecov.io/gh/apache/libcloud/pull/1397/diff?src=pr&el=tree#diff-bGliY2xvdWQvdGVzdC9jb21wdXRlL3Rlc3RfZWMyLnB5) | `97.97% <100%> (+0.01%)` | :arrow_up: |
   | [libcloud/compute/drivers/ec2.py](https://codecov.io/gh/apache/libcloud/pull/1397/diff?src=pr&el=tree#diff-bGliY2xvdWQvY29tcHV0ZS9kcml2ZXJzL2VjMi5weQ==) | `75.27% <100%> (+0.06%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/libcloud/pull/1397?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/libcloud/pull/1397?src=pr&el=footer). Last update [1ea7b36...1420a55](https://codecov.io/gh/apache/libcloud/pull/1397?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services