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
DM-38742: Enable use of Ceph multi-tenant bucket names. #54
Conversation
830c6db
to
389d5a1
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #54 +/- ##
==========================================
+ Coverage 85.74% 85.84% +0.10%
==========================================
Files 27 27
Lines 3816 3836 +20
Branches 785 789 +4
==========================================
+ Hits 3272 3293 +21
Misses 426 426
+ Partials 118 117 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
81e6383
to
7e0c225
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay in theory but I'd like a different name and I worry about env var existence vs truth/false value.
python/lsst/resources/s3utils.py
Outdated
@@ -147,6 +148,10 @@ def getS3Client() -> boto3.client: | |||
----- | |||
The endpoint URL is from the environment variable S3_ENDPOINT_URL. | |||
If none is specified, the default AWS one is used. | |||
|
|||
If the environment variable LSST_CEPH_BUCKETS exists (with any value), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned that CEPH is not the only reason that someone would want to do this. How about calling it something like LSST_S3_DISABLE_VALIDATION
? Also, I'm not sure I like it that we don't have to set it to a truthy value.
@@ -82,6 +85,16 @@ def testBucketExists(self): | |||
self.assertTrue(bucketExists(f"{self.bucketName}")) | |||
self.assertFalse(bucketExists(f"{self.bucketName}_no_exist")) | |||
|
|||
def testCephBucket(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this test fail if someone has set the environment variable in the shell?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay. Thanks.
Checklist
doc/changes