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

Don’t configure Kamal storage volume if not needed #51838

Merged

Conversation

jeromedalbert
Copy link
Contributor

@jeromedalbert jeromedalbert commented May 15, 2024

Motivation / Background

Fixes #51836.

This Pull Request has been created because configuring a Docker persistent storage volume in Kamal seems to be only needed for sqlite or Active Storage. If using a different database and the --skip-active-storage option, configuration can be skipped.

Detail

This Pull Request changes the config/deploy.yml.tt template to skip persistent storage volume configuration if skipping Active Storage and using a database that is different from sqlite.

Additional information

The condition I implemented is the same as the one used here in the rails app generator.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@yahonda
Copy link
Member

yahonda commented May 16, 2024

Would you add a test to make sure this option works as expected? I think some tests at https://github.com/rails/rails/blob/main/railties/test/generators/app_generator_test.rb are helpful as a reference.

@jeromedalbert jeromedalbert force-pushed the no-kamal-storage-volume-if-not-needed branch from 2f089cb to dccb36f Compare May 17, 2024 04:44
@jeromedalbert
Copy link
Contributor Author

@yahonda Done, thanks for the pointer!

@jeromedalbert jeromedalbert force-pushed the no-kamal-storage-volume-if-not-needed branch from dccb36f to bc220cd Compare May 17, 2024 22:53
@jeromedalbert jeromedalbert force-pushed the no-kamal-storage-volume-if-not-needed branch 2 times, most recently from 824a875 to 2db1d4c Compare May 18, 2024 20:48
Configuring a persistent storage volume in Kamal
is needed only for sqlite or Active Storage. If
using a different database or the
--skip-active-storage option, this configuration
can be skipped.
@jeromedalbert jeromedalbert force-pushed the no-kamal-storage-volume-if-not-needed branch from 2db1d4c to 3156e50 Compare May 20, 2024 01:31
@jeromedalbert
Copy link
Contributor Author

It seems like the buildkite failed docs-preview job is unrelated.

@yahonda yahonda merged commit 5c92d45 into rails:main May 20, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Storage folder sometimes doesn't need Kamal configuration in Rails 8.0.0.alpha
3 participants