Tbox 73 cloud transfer

[DIvkov575]

↪️ Pull Request

✔️ PR Todo

• Add documentation
• Add examples
• Ensure you are following the practices in the contributor guide and coding_standards
  • Use consistent and descriptive name conventions
  • Uses google-style docstrings
  • Uses versioned API/client methods only
  • Avoid global variables
  • Avoid giant try/catch
  • Don't use mutable default variables
  • Use a main() function in your scripts
• Added/updated testing for this change (ensure you are testing any changes/new code!)
• Update title to link to TBOX issue (i.e. start pull request title with TBOX-xx)
• Approval from first code-reviewer
• Approval from second code-reviewer (only required if a second reviewer has left comments)
• Pass all checks

[ycharm]

Hi we had a docs bug that was blocking the tests from passing, can you merge in develop to your PR. It will allow the tests to pass. Thanks!

[DIvkov575]

@jun-dao What would you recommend for testing google & AWS uploading/downloading? Thank you.

[ycharm]

hi any progress on trying to do testing? Can I help in any way?

[DIvkov575]

For now, I'm just looking into the boto3 and botocore libraries. Im not sure whether to try and use moto for mocking because it is not a tamr dependency.

[DIvkov575]

@ycharm Im getting an issue with asserting that a temp file is a tarfile. Any recommendations for getting around this?
Is there anything else I should add to the test?

[ycharm]

@ycharm Im getting an issue with asserting that a temp file is a tarfile. Any recommendations for getting around this? Is there anything else I should add to the test?
Is this helpful: https://stackoverflow.com/questions/23883947/combining-tempfile-and-tarfile-with-python

[DIvkov575]

@ycharm I'm realizing that the test would not work through trying to unit test mock it. The mock does not return anything so asserting is impossible on an empty temp file.

I've still been unable to find any alternative default credentials or mocking for GCP, but have an idea for testing.

testing tarring could be done by adding an optional return to the cloud_upload function (returns file about to be uploaded).
Is this a viable option for testing?

[ycharm]

@DIvkov575 I don't see anything in our guidelines that suggests we shouldn't have the optional return. Especially if you need it for testing. @clint-richardson check out the question please.

[DIvkov575]

@skalish are there any other changes that I should make?

[skalish]

There are a couple of dependency definitions to be taken care of. The testing should also be reworked to be a bit cleaner and more complete. I think those will be the last needed changes. Let me know if I can help at all.

[skalish]

After you resolve the one accidental import removal, this is good to go. I also asked a question about the need for the google package. If you can follow-up on that it would be nice to confirm whether it is necessary, but that isn't absolutely needed. I'll approve as soon as I can once these last changes are made.

[DIvkov575]

After you resolve the one accidental import removal, this is good to go. I also asked a question about the need for the google package. If you can follow-up on that it would be nice to confirm whether it is necessary, but that isn't absolutely needed. I'll approve as soon as I can once these last changes are made.

I think that the google import / install is necessary because inorder to use the google-cloud-storage client you need to import it with from google.cloud import storage and without google installed it wont be able to find where to import from

[skalish]

We shouldn't spend more time digging in on the google package question. It will be very easy to remove if needed in the future. This all looks good to me! Thanks for all of the work you've put it on this.

Let me know if you'd like me to click to merge this in for you when you're ready.

[DIvkov575]

@skalish yes, could you merge this. I don't have a merge button.