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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise exception if Active Storage is configured and config.active_storage.service is not explicitly set #44372

Merged
merged 1 commit into from May 31, 2022

Conversation

ghousemohamed
Copy link
Contributor

@ghousemohamed ghousemohamed commented Feb 9, 2022

Summary

If config.active_storage.service has not been explicitly set in the respective environment's configuration file, then trying to use active storage throws a vague error message, which makes it harder to debug. This was the following error I had gotten:

Failed to replace attachments_attachments because one or more of the new records could not be saved.

Turned out I had forgotten to explicitly specify config.active_storage.service in my configuration file. Specifying it resolved the issue.

I had spent a considerable amount of time trying to debug this, and I would have immediately known what the fix was if the error message had been clearer. So, I'm turning in this PR so that anyone facing this in the future will know what to do at a quick glance.

Thanks 馃榿!

Other Information

@ghiculescu
Copy link
Member

Hmm, based on the failing tests, it might be better if this error is raised when Active Storage is used. Currently this will raise on any Rails app that has Active Storage loaded (which it does by default) but doesn't have the config, even if it never actually uses AS.

Copy link
Contributor

@jorgemanrubia jorgemanrubia left a comment

Choose a reason for hiding this comment

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

Thanks @ghousemohamed, we'll get this merged.

@jorgemanrubia
Copy link
Contributor

@ghousemohamed could you please solve the merge conflicts here?

@ghousemohamed
Copy link
Contributor Author

ghousemohamed commented May 31, 2022

@jorgemanrubia I've resolved the merge conflicts

@jorgemanrubia
Copy link
Contributor

@ghousemohamed thanks for sorting those out. A last request: could you add an entry to activestorage/CHANGELOG.md for the change 馃檹?

@ghousemohamed
Copy link
Contributor Author

@ghousemohamed thanks for sorting those out. A last request: could you add an entry to activestorage/CHANGELOG.md for the change 馃檹?

@jorgemanrubia Done

@jorgemanrubia
Copy link
Contributor

Thanks @ghousemohamed. This is good to merge @jeremy 馃檹

Copy link
Member

@jeremy jeremy left a comment

Choose a reason for hiding this comment

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

Nice. Thanks @ghousemohamed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants