Skip to content
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

Promote `ex_prefix` to top-level storage SDK argument #1397

Merged
merged 16 commits into from Jan 9, 2020

Conversation

@c-w
Copy link
Member

c-w commented Dec 26, 2019

Promote ex_prefix to top-level storage SDK argument

Description

Most storage drivers implement ex_prefix for list_container_objects and iterate_container_objects. As such, this pull request promotes ex_prefix to a top-level argument. For each driver that didn't previously implement ex_prefix, a fallback to filter objects client-side is has also been added.

Status

  • done, ready for review

Checklist

  • Code linting (build passed)
  • Documentation (updated existing docstrings)
  • Tests (updated existing tests)
  • ICLA (committer)
@c-w c-w added the api: storage label Dec 26, 2019
@c-w c-w force-pushed the CatalystCode:storage_ex_prefix branch from 4e92b60 to d1b219a Dec 26, 2019
@Kami

This comment has been minimized.

Copy link
Member

Kami commented Dec 26, 2019

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.

@c-w

This comment has been minimized.

Copy link
Member Author

c-w commented Dec 28, 2019

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

@Kami

This comment has been minimized.

Copy link
Member

Kami commented Dec 28, 2019

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

@c-w

This comment has been minimized.

Copy link
Member Author

c-w commented Dec 29, 2019

Thanks for the additional context @Kami. I'll work on the change and update this PR.

@c-w c-w force-pushed the CatalystCode:storage_ex_prefix branch from d1b219a to eb73e76 Dec 29, 2019
@c-w c-w changed the title Enable `ex_prefix` to be passed via all storage SDK methods Promote `ex_prefix` to top-level storage SDK argument Dec 29, 2019
@c-w c-w force-pushed the CatalystCode:storage_ex_prefix branch from eb73e76 to f3596ec Dec 29, 2019
c-w added 2 commits Dec 29, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 29, 2019

Codecov Report

Merging #1397 into trunk will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            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 Coverage Δ
libcloud/test/compute/test_ec2.py 97.97% <100%> (+0.01%) ⬆️
libcloud/compute/drivers/ec2.py 75.27% <100%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ea7b36...1420a55. Read the comment docs.

@c-w c-w requested a review from Kami Dec 30, 2019
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)

This comment has been minimized.

Copy link
@Kami

Kami Jan 8, 2020

Member

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.

@Kami
Kami approved these changes Jan 8, 2020
Copy link
Member

Kami left a comment

Nice work, LGTM 👍

When you get a chance it would also be good to add upgrade notes entry (document this new argument and advise users to migrate from ex_prefix to prefix if they currently use ex_prefix).

Kami and others added 3 commits Jan 8, 2020
@c-w

This comment has been minimized.

Copy link
Member Author

c-w commented Jan 9, 2020

Added upgrade note in 16c692f.

@c-w c-w merged commit f176f5b into apache:trunk Jan 9, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@c-w c-w deleted the CatalystCode:storage_ex_prefix branch Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.