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

[s3] raise ImproperlyConfigured if no bucket name is set #1313

Merged
merged 1 commit into from
Sep 29, 2023

Conversation

jschneier
Copy link
Owner

Closes #1140

@jschneier jschneier merged commit 9eeb8c3 into master Sep 29, 2023
15 checks passed
@jschneier jschneier deleted the josh/s3bucket-exception branch September 29, 2023 05:20
@banagale
Copy link

banagale commented Oct 6, 2023

This change broke my testing setup in CI.

I'm not disputing that this is a correct change to the package, only that it was a breaking change for how I was loose with s3 variable definitions in my CI.

A while back I made a choice on this project not to use S3 when running unit tests.

On my my local machine, running tests manually, I defined the bucket.

But in CI I was setting AWS_STORAGE_BUCKET_NAME to None and getting away with it.

Looking at it again, I see I've had to make a number of small hacks to handle not using S3 there.

With this new error, it seems like maybe its time to clear the cruft and just let CI have at a valid django-storages with s3 setup.

Are people reading / writing to S3 in their unit tests?

Could be I'm not configuring django-storages for non-s3 usage in my CI run.

@mike667
Copy link

mike667 commented Oct 6, 2023

@jschneier Hello, I have same issue as @banagale. ImproperlyConfigured error broke my CI. I fixed it by set random bucket name in my CI, but is there a way to move this check in another place or option to disable this check?
Thanks

@banagale
Copy link

banagale commented Oct 6, 2023

@mike667 Glad I'm not the only one. Can you give more detail on the context of how you're using django-storages?

Also, curious if you have a reason for needing S3 in your CI, is it part of unit testing?

This particular issue popped up due to a reliance on the package from a wagtail installation I did recently.

@jschneier
Copy link
Owner Author

Hi in general I would expect that CI and Test would swap out the backend but I’ll see what I can do.

@banagale
Copy link

banagale commented Oct 7, 2023

Hi in general I would expect that CI and Test would swap out the backend but I’ll see what I can do.

I think swapping out the backend for CI and test is the right way to go about it.

So, I don't know if a change is needed. The docs clearly say bucket names are required.

It could be that adding guidance on config for unit testing in the docs would be enough. Right now there are no results for Unit Tests searching the docs. If helpful, I could file an issue on this and look at providing a PR.

jschneier added a commit that referenced this pull request Oct 9, 2023
@jschneier
Copy link
Owner Author

I am reverting this commit and will release a new version as soon as this and the other pr land.

jschneier added a commit that referenced this pull request Oct 9, 2023
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.

Not setting bucket name causes confusing error
3 participants