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

Use S3 (MinIO emulator) as storage backend #7812

Merged
merged 1 commit into from Jan 26, 2021

Conversation

humitos
Copy link
Member

@humitos humitos commented Jan 8, 2021

  • remove Azurite storage backend
  • update all settings to use S3
  • update docs to mention set "Read Only" on buckets
  • disable S3 console logs
  • bring s3_storage.py from -ext
  • delete azure_storage.py from .org
  • update django-storages to install boto3 dependencies

@humitos humitos requested a review from a team January 8, 2021 16:09
@humitos humitos force-pushed the humitos/docker-env-s3-storage branch from 656fceb to dba9483 Compare January 21, 2021 10:33
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

Seems to look good to me!

Comment on lines 23 to 24
SLUMBER_USERNAME = 'admin'
SLUMBER_PASSWORD = 'admin'
Copy link
Contributor

Choose a reason for hiding this comment

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

These look to be new, are these settings intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

These are required for builds to hit the API. Otherwise, we get 403 and builds don't work.

This problem is not new, but I suppose that you may have these settings overwritten in a local_settings.py file?

They are required in docker environment to be able to build docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can remove them from this PR and figure it out in another one, tho.

Copy link
Member

Choose a reason for hiding this comment

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

Yea, seems odd this would be related to this PR. It must have been working before somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's completely unrelated to this PR, yes. I will remove it from it and add it to another one so we can discuss it there. We will see if it fails for you as well when building.

# in our Docker setup. We can upgrade it but we need to add a
# `create_buckets.sh` to be called on `--init` as we used to do for Azurite
# https://github.com/jschneier/django-storages/pull/636
git+https://github.com/jschneier/django-storages@d0f027c98a877f75615cfc42b4d51c038fa41bf6#egg=django-storages[boto3]==1.9.1
Copy link
Contributor

Choose a reason for hiding this comment

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

We can track this with a separate issue perhaps, we probably don't want to get too far behind on django-storages releases

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure to follow you here.

We are not changing the version in this PR. I just changed azure by boto3 + add a comment explaining why we can't upgrade.

raise ImproperlyConfigured(
'AWS S3 not configured correctly. '
'Ensure S3_BUILD_ENVIRONMENT_STORAGE_BUCKET is defined.',
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: we'll want a PR on commercial/ops/etc in the near future to make sure we're using this class instead of the ext class

Also, I assumed this file is just forked and didn't spend much time at all on review here.

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This looks good to me. Will be good to get it testing locally 👍

Comment on lines 23 to 24
SLUMBER_USERNAME = 'admin'
SLUMBER_PASSWORD = 'admin'
Copy link
Member

Choose a reason for hiding this comment

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

Yea, seems odd this would be related to this PR. It must have been working before somehow?

- remove Azurite storage backend
- update all settings to use S3
- update docs to mention set "Read Only" on buckets
- disable S3 console logs
- bring `s3_storage.py` from -ext
- delete `azure_storage.py` from .org
- update django-storages to install boto3 dependencies
@humitos humitos force-pushed the humitos/docker-env-s3-storage branch from dba9483 to 39af6e8 Compare January 26, 2021 09:54
@humitos humitos merged commit 76e0d3d into master Jan 26, 2021
@humitos humitos deleted the humitos/docker-env-s3-storage branch January 26, 2021 10:06
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

3 participants