Skip to content
This repository has been archived by the owner on Jun 5, 2023. It is now read-only.

Update storage client to handle retries and explicit credentials. #632

Merged
merged 6 commits into from Sep 11, 2017

Conversation

ahoying
Copy link
Collaborator

@ahoying ahoying commented Sep 11, 2017

  • Set file_loader util to use an explicit credential object so that
    per thread http object caching is not used for loading and saving files
    to a GCS bucket.

Thanks for opening a Pull Request!

Here's a handy checklist to ensure your PR goes smoothly.

  • I signed Google's Contributor License Agreement
  • My code conforms to Google's python style.
  • My PR at a minimum doesn't decrease unit-test coverage (if applicable).
  • My PR has been functionally tested.
  • All of the unit-tests still pass.
  • Running pylint --rcfile=pylintrc passes.

These guidelines and more can be found in our contributing guidelines.

* Set file_loader util to use an explicit credential object so that
per thread http object caching is not used for loading and saving files
to a GCS bucket.
@codecov
Copy link

codecov bot commented Sep 11, 2017

Codecov Report

Merging #632 into dev will not change coverage.
The diff coverage is 90%.

@@           Coverage Diff           @@
##              dev     #632   +/-   ##
=======================================
  Coverage   83.33%   83.33%           
=======================================
  Files         164      164           
  Lines        8103     8109    +6     
=======================================
+ Hits         6753     6758    +5     
- Misses       1350     1351    +1
Impacted Files Coverage Δ
google/cloud/security/common/gcp_api/storage.py 100% <100%> (ø) ⬆️
google/cloud/security/common/util/file_loader.py 88.46% <87.5%> (-0.67%) ⬇️

Copy link
Contributor

@blueandgold blueandgold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a qq on the change here. Thanks for helping me to understand what I might be mising.

"""
# Pass credential in explicitly so that cached credentials are not used.
credentials = client.GoogleCredentials.get_application_default()
storage_client = storage.StorageClient(credentials=credentials)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sure I am missing something, so please help me to understand how this is different from the previous existing code.

So before, we create the storage_client like this:
storage_client = storage.StorageClient()

Which in turn initialize the super _base_repository.BaseRepositoryClient()

The super also end up getting the application default credential in the init().
https://github.com/GoogleCloudPlatform/forseti-security/blob/dev/google/cloud/security/common/gcp_api/_base_repository.py#L130

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is the setting of the internal _use_cached_http flag, which is only set to True when credentials are not passed in. By passing in credentials it forces the client to generate a new http object for each request instead of using a cached one. That's the behavior we want for the file_loader.

Now this is not the only way that could be accomplished, but it does require the least code changes. If you think it's too confusing I can look at a deeper refactor of the logic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ahoying ahoying merged commit 4b0852b into dev Sep 11, 2017
@ahoying ahoying deleted the storage-fix branch September 11, 2017 23:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants