add requester pay to s3 download and upload if in config settings #82
Conversation
sliu008
commented
Aug 19, 2025
sliu008
commented
- Added in requester pay to s3 download and upload function if it is in the config settings
dmrpp_generator/main.py
Outdated
| def _get_s3_extra(self): | ||
| """Helper to build the extra dict for S3 operations.""" | ||
| extra = {} | ||
| if self.config.get("requester_pay", False): |
There was a problem hiding this comment.
Do we need to parse the value as a boolean here?
| if self.config.get("requester_pay", False): | |
| if f'{self.config.get("requester_pay", False)}.lower() == 'true': |
or is the value in self.config already a boolean?
There was a problem hiding this comment.
i just tested the value is already a boolean
dmrpp_generator/main.py
Outdated
| def _get_s3_extra(self): | ||
| """Helper to build the extra dict for S3 operations.""" | ||
| extra = {} | ||
| if self.config.get("requester_pay", False): |
There was a problem hiding this comment.
Would it be possible to update this config location to align with the other dmrpp flags like get_dmrpp_timeout i.e. the config.collection.meta.dmrpp (from collection) or config.dmrpp (from workflow)
The terraform variable is likely not needed for this PR, but it would be nice to keep these dmrpp specific variable flags consolidated to one location.
For this PR, it would just mean updating self.config.get to self.dmrpp_meta.get
There was a problem hiding this comment.
It’s technically possible, but I’m not sure it makes sense. This seems like it would apply on a per-collection basis, whereas what we actually need is for it to depend on where the DMRPP generator runs relative to the bucket location. Our goal is to enable requester pays specifically for cross-account S3 requests. If i were to do it on the collection config that means i would need to modify my collection configuration based on which env were running the dmrpp generator on.
I think i would prefer it on the config level where it is based on the step function workflow setup.
There was a problem hiding this comment.
ahh ok i see, so in the dmrpp override in the workflows, i can work that in.
3 participants
