-
Notifications
You must be signed in to change notification settings - Fork 128
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
Add option to specify a prefix to be prepended to object paths #190
Conversation
Hi @hoffmanr-cshs , thank you so much for your contribution! |
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.
Thanks for your patience while I took a look at this. I think this change works really well with STRIP_LEADING_DIRECTORY_PATH
as you mentioned in the description. Having the ability to strip, prepend, or replace initial object paths is great flexibility.
I have a few comments:
-
What do you think about choosing a name for the configuration that more closely groups the two configuration options? Maybe something like
PREPEND_LEADING_DIRECTORY_PATH
? Your name matches the S3 terminology better but I'm trying to group configuration options as much as possible. -
I have a suggestion for an alternate approach to adding the prefix. See comment in code
-
Adding tests for this one is a little tricky since it prefixes everything and out integration test suite is in need of some love. I think this PR will be what pushes me to finally refactor it into javascript but for now I can help add some basic tests to this PR in a hacky way. I can add those once we decide on an approach for (2) and (3)
…a bucket Thank you to @4141done for the suggestions! (nginxinc#190)
Thanks for your reply - and no rush! To your points:
I've taken a pass at implementing and validating these changes in my application, and it all seems to be working well! I've just pushed a reversion of s3gateway.js and a commit with the suggested changes - take a look, and me know if you have any more thoughts! |
Once the tests are in I think we are good to merge. My final request to you would be to test the current iteration of this PR against your use case to make sure it still works for you after the change in approach. |
@hoffmanr-cshs tests are in, please let me know if you think I missed anything critical. At this point once you give me the 👍 that you are happy with how this looks we will merge 🐳 |
all looks great to me - thank you! |
All done! Thank you once again for your contribution! |
This draft PR adds an optional configuration environment variable
S3_OBJECT_PREFIX
. If defined, this prefix is prepended to all S3 object paths. This is intended as a simple way to allow only some subset of a bucket to be served by the gateway. When used in combination with the existingSTRIP_LEADING_DIRECTORY_PATH
option, this effectively allows the leading prefix to be replaced, rather than just removed.I don't know if this will be of value to any others, or if there are potentially any "gotchas" to be aware of implementing it in this way. But just in case it's useful for anyone, I wanted to share!