Use mc instead of aws

[benjaoming]

• Use the mc MinIO client instead of aws, since MinIO policy changes are not supported through aws client.
• Add wait-for-it.sh for web (waiting for database, conditionally waiting for storage) and celery (conditionally waiting for search)
• Remove wait_for_search.py

Fixes:

• bucket policies not created, had to be edited manually after creation.

[humitos]

This was the reason why bucket policies had to be edited manually after creation.

Did you need to edit anything manually? AFAIK, we removed this requirement when we migrated to the usage of aws to create the buckets and this is now done automatically when using --init the first time.

See readthedocs/readthedocs.org@af217d0

If possible, I'd prefer to not change how this works and solve the issue with the permissions if that's an issue at all. One of the benefits of MinIO is that it's compatible with the AWS API and we can use standard tools with it --if we change the MinIO backend for another that's compatible with AWS API we won't need to upgrade our code, that's 💯

[benjaoming]

I found that the old script was running fine, but policies were set as "Private". This is likely because MinIO doesn't implement this part of the S3 API. My guess is that when running aws, the --acl option is ignored.

See: "List of Amazon S3 Bucket API's not supported on MinIO", specifically "BucketACL (Use bucket policies instead)" here: https://docs.min.io/docs/minio-server-limits-per-tenant.html

[stsewd]

Did you need to edit anything manually? AFAIK, we removed this requirement when we migrated to the usage of aws to create the buckets and this is now done automatically when using --init the first time.

That stopped working some time ago, I think it was when minion updated their UI

[benjaoming]

Reminder to update readthedocs/readthedocs.org#9319 once this is merged

[benjaoming]

I've now added a call to wait-for-it.sh because the container all to frequently started executing its entrypoint before the MinIO storage container was ready.

[benjaoming]

(I prefer to inject the script to the alternative of rebuilding the minio/mc image with similar capabilities, however for our own Ubuntu-based images, we can just add the package wait-for-it)

[humitos]

Looks good to me!

I'd like to not spin up an extra container for creating buckets if possible. I'm proposing a simpler solution by re-using the storage one and following the pattern we already have for other containers (like web)

Besides that, I left some other small suggestions as well.

[humitos]

Looks good to me! We need to handle the "wait for it" for the Celery container when we are using --no-search before merging, I guess.

[humitos]

Changes look good! However, I think this PR deviates too much from the original purpose and got too complex due to the introduction of wait-for-it --which does not seem strictly required.

I'd prefer if we keep this PR simple and only introduce the required changes for mc which was the original purpose. I'm sorry 🙏🏼 if I added confusion in my review comments, but originally it looked like a straightforward change and it ended up not being trivial and breaking search on the application.

[benjaoming]

Tested that containers are waiting for each other and --no-search option works ✔️
Rewrote the commit history ✔️

[humitos]

Looks good to me! 💯

Pinging @stsewd who is the other person heavily using the Docker development setup in case he has opinions on this.

[stsewd]

Just realized that we were expecting to create all buckets as public before this change, but I think we should try to match production and set some as private. But I'm fine doing that in another PR.

[humitos]

I just realized that we can remove this Python dependency now that it's not gonna be used anymore, https://github.com/readthedocs/readthedocs.org/blob/51de8840cc23a62150c9ad1ac6f1999b87402fd6/requirements/docker.txt#L22-L23