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

DM-41879: Implement S3 URL signing #73

Merged
merged 6 commits into from Dec 8, 2023
Merged

DM-41879: Implement S3 URL signing #73

merged 6 commits into from Dec 8, 2023

Conversation

dhirving
Copy link
Contributor

@dhirving dhirving commented Dec 5, 2023

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes

@dhirving dhirving changed the title Tickets/dm 41879 DM-41879: Handle S3 URL signing Dec 5, 2023
Copy link

codecov bot commented Dec 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d2ae093) 86.10% compared to head (60ba628) 86.10%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #73      +/-   ##
==========================================
- Coverage   86.10%   86.10%   -0.01%     
==========================================
  Files          27       27              
  Lines        4030     4065      +35     
  Branches      826      831       +5     
==========================================
+ Hits         3470     3500      +30     
- Misses        438      446       +8     
+ Partials      122      119       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dhirving dhirving force-pushed the tickets/DM-41879 branch 2 times, most recently from 229593b to d657d99 Compare December 5, 2023 21:21
@dhirving dhirving changed the title DM-41879: Handle S3 URL signing DM-41879: Implement S3 URL signing Dec 5, 2023
@dhirving dhirving force-pushed the tickets/DM-41879 branch 2 times, most recently from 9a11f9e to 800032e Compare December 5, 2023 22:54
Added a new function for cleaning up the S3 environment before running tests.  This replaces the two existing functions which had some problems:
* 'clean_test_environment' wasn't clearing every environment variable that could affect the tests.
* 'setAwsEnvCredentials' was hazardous because it could leave real credentials in place by accident, causing real infrastructure to be mutated.
Limit the parent directory creation in HttpResourcePath.write() to WebDAV endpoints only.  Presigned S3 URLs do not have any way to check for existence of the parent directory.  S3 has no concept of directories and even if it did, you wouldn't be able to do anything about them via a URL signed for the child.
unittest.TestCase.enterContext() only became available in 3.11
@dhirving dhirving marked this pull request as ready for review December 6, 2023 17:33
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning up the tests.

@@ -0,0 +1 @@
Added functions to S3ResourcePath for generating presigned HTTP URLs
Copy link
Member

Choose a reason for hiding this comment

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

I think you have added a new API to ResourcePath (say what it is) and there is only an implementation for S3.

python/lsst/resources/_resourcePath.py Outdated Show resolved Hide resolved
python/lsst/resources/_resourcePath.py Outdated Show resolved Hide resolved
python/lsst/resources/_resourcePath.py Outdated Show resolved Hide resolved
python/lsst/resources/http.py Outdated Show resolved Hide resolved
python/lsst/resources/s3utils.py Show resolved Hide resolved
Tim decided we should have a period of deprecation for these before removing them
@dhirving dhirving merged commit a713f88 into main Dec 8, 2023
15 of 16 checks passed
@dhirving dhirving deleted the tickets/DM-41879 branch December 8, 2023 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants