Closed bolliger32 closed 4 years ago
@delgadom @brews I updated this so that it will handle the version of google-cloud-storage
currently installed on the servers, in addition to the latest version. It should be ready to go. We don't have any tests for the gcs
commands yet. I didn't make one for this new rm
command either, but we should probably start doing this at some point...
@bolliger32 IMHO, the way to test the rhg_compute_tools.gcs is by mocking the various google cloud modules/functions/classes. It'd be a white-box test so unless you write the unit tests for the new functionality in this PR, then maybe this is something to do once we're sure that we are handling our interactions with GCS the way we want -- otherwise we'd have to change the unit tests every time we change how we interact with GCS.
^ Just my two cents. I'm out in the UK working with James all next week but I'm happy to review/support in any way I can.
@brews sounds good. I'm terrible at writing tests (classic academic programmer...) but I'm happy to make a unit test for rm
if that would be helpful. In order for this to work, we'd need to give travis access to our (or at least some) GCS bucket, correct? Seems like a fairly significant task but something we should do at some point...I'd be in favor of getting this merged in and putting that in a separate issue, but either way is good with me! We need this rm
functionality for our surge modeling but we're just working off of this branch for now
@bolliger32 Yeah, that's the tricky bit. You wouldn't want to give Travis access to GCS. Instead you'd have to mock GCS for the tests.
Edit:
After a quick google, here is a tutorial on pytest + pytest-mock https://codefellows.github.io/sea-python-401d7/lectures/mock.html (not sure if its any good, but to point you in a direction)
Cool ya totally makes sense
On Fri, Sep 20, 2019 at 9:53 AM Brewster Malevich notifications@github.com wrote:
@bolliger32 https://github.com/bolliger32 Yeah, that's the tricky bit. You wouldn't want to give Travis access to GCS. Instead you'd have to mock GCS for the tests.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/RhodiumGroup/rhg_compute_tools/pull/41?email_source=notifications&email_token=ABEUHFW6P54DXLKIB77BEXTQKT5ZRA5CNFSM4IN7AP22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7HJCXI#issuecomment-533631325, or mute the thread https://github.com/notifications/unsubscribe-auth/ABEUHFWMKOA5M4Q7XUUPIH3QKT5ZRANCNFSM4IN7AP2Q .
-- Sent from Gmail Mobile
@delgadom @brews assuming we're not planning on implementing tests for the gcs functions yet, this should be ready to merge
If that's the case then I'd say merge & deploy it!
So this is forward compatible with RhodiumGroup/docker_images#102 but not with the current deployment right? But anyone could make their environment compatible with this version by adding GOOGLE_APPLICATION_CREDENTIALS to their worker-template.yml file?
It is compatible with the current deployment I believe. Right now it assigns the credentials filepath to both GOOGLE_APPLICATION_CREDENTIALS
and GCLOUD_DEFAULT_TOKEN_FILE
. Once RhodiumGroup/docker_images#101 is merged, we can drop GCLOUD_DEFAULT_TOKEN_FILE
altogether. Merging now!
flake8 rhg_compute_tools tests docs
Add an
rm
functionality to thegcs
module.