-
Notifications
You must be signed in to change notification settings - Fork 678
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
Update the recommended s3 policy in s3 offload media #8731
Conversation
⚡ Deployed with Pantheon Decoupled This build was successfully deployed with Pantheon. You can track the build logs here. 👀 Preview: https://pr-8731-documentation.appa.pantheon.site |
@carl-alberto who else can review the accuracy of the change? I don't have the subject matter expertise to do so myself. |
@stevector hi! I have verified that these settings do work. I tested on my sandbox site, and the files are directly offloaded to the S3 bucket. It should be noted that this PR removes access to list the buckets, which is an option with the WP Offload Media plugin. Can we rely on customers to copy/paste their bucket name appropriately for configuration? Since the bucket listing is a feature in the plugin, this gives me pause at face value. Also, the screenshot needs to be updated. If so, I'd say this is a pass (save for the screenshot). If not, I can play around with the permissions and get the bucket listing going. |
I've never used this plugin, but I just based on the level of prescriptive detail already in this page of Docs, I think that if we tell the reader that they need to copy/paste the bucket name that it'll be fine. If you're trying this out on your own S3, can you take updated screenshots? |
Ok, great! I wasn't sure.
Absolutely. I'll get that in tomorrow and tag you here/in Slack. |
@stevector As I'm not a docs admin, I haven't actually hit the review button, but I've added the updated screenshot - and added a note where it talks about using the full S3 access permission regarding needing that if you want to list buckets. LGTM now. Off to you. 🙂 |
I'm removing the old |
I'm not sure why deployments are failing here, investigating. |
⚡ Deployed with Pantheon Decoupled This build was successfully deployed with Pantheon. You can track the build logs here. 👀 Preview: https://pr-8731-documentation.appa.pantheon.site |
Thanks @miriamgoldman!! |
Closes #8730
Summary
Reviewed the policy outlined, it seems it is outdated as reported by 2 customers
Effect
The following changes are already committed:
Remaining Work and Prerequisites
The following changes still need to be completed:
Dependencies and Timing
Release:
Post Launch
Do not remove - To be completed by the docs team upon merge:
/old-path/
=>/new-path/
(if applicable)