Ensure the RUN_AS ownership applies to the DB's named volume #2099
Conversation
See conversation [here](sartography#2096 (comment)). Tested locally; it works!
📝 Walkthrough📝 WalkthroughWalkthroughThe Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
docker-compose.yml (1)
16-24: LGTM with suggestions for improvementThe
match-volume-ownershipservice effectively ensures that theRUN_ASuser has write permissions to the SQLite DB volume. This is a good solution to the volume ownership issue.Suggestions for improvement:
- Consider adding a condition to check if the ownership change is necessary before executing
chown. This could potentially speed up container startup times for subsequent runs.- While running as root is necessary for changing ownership, consider adding the
--userflag to thechowncommand to explicitly run as root, making the intention clear.Example implementation:
match-volume-ownership: image: alpine restart: "no" user: root entrypoint: | /bin/sh -c " if [ \"$(stat -c '%u:%g' /app/db_volume)\" != \"${RUN_AS:-0:0}\" ]; then chown ${RUN_AS:-0:0} /app/db_volume else echo 'Ownership is already correct. Skipping.' fi " volumes: - spiffworkflow_backend_db:/app/db_volume:rwThis change would only update the ownership when necessary and explicitly runs as root.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- docker-compose.yml (1 hunks)
🔇 Additional comments (2)
docker-compose.yml (2)
32-33: LGTM: Correct dependency addedThe addition of the
depends_onclause for thematch-volume-ownershipservice is correct and necessary. This ensures that the volume ownership is set properly before thespiffworkflow-backendservice starts, which is crucial for the proper functioning of the application.
Line range hint
1-85: Summary: PR objectives achieved with minor suggestions for optimizationThis PR successfully addresses the objective of ensuring that the
RUN_ASownership applies to the DB's named volume. The implementation:
- Adds a new
match-volume-ownershipservice to set the correct ownership of the volume.- Updates the
spiffworkflow-backendservice to depend on the new service.These changes should resolve the issue mentioned in the previous PR (#2096). The implementation is correct and should work as intended, aligning with the author's statement that it has been tested locally.
Minor optimizations have been suggested for the
match-volume-ownershipservice, but these are not critical for the functionality. The current implementation is approved and ready for merging, pending any final thoughts from @burnettk or other reviewers.
See helpful comment [here](docker/compose#3270 (comment)).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
docker-compose.yml (1)
16-24: LGTM! Consider adding a comment about the RUN_AS variable.The new
match-volume-ownershipservice is well-implemented to ensure proper ownership of the database volume. It correctly uses theRUN_ASenvironment variable and sets up the volume mapping consistently with thespiffworkflow-backendservice.Consider adding a comment explaining the default value of
RUN_AS, similar to the one in thespiffworkflow-backendservice:match-volume-ownership: image: alpine restart: "no" entrypoint: | + # Use RUN_AS if set, otherwise default to root (0:0) /bin/sh -c "chown ${RUN_AS:-0:0} /app/db_volume" volumes: - spiffworkflow_backend_db:/app/db_volume:rwThis addition would improve consistency and clarity across the configuration.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- docker-compose.yml (1 hunks)
🔇 Additional comments (2)
docker-compose.yml (2)
32-34: LGTM! Dependency on match-volume-ownership is correctly implemented.The addition of the
depends_onclause with theservice_completed_successfullycondition ensures that thespiffworkflow-backendservice starts only after the volume ownership has been properly set. This implementation aligns perfectly with the PR objective.
Line range hint
1-85: Overall, the changes effectively address the PR objective.The modifications to the
docker-compose.ymlfile successfully implement a solution to ensure that theRUN_ASownership applies to the DB's named volume. The newmatch-volume-ownershipservice and the updated dependency in thespiffworkflow-backendservice work together to achieve this goal without introducing unnecessary complexity.These changes should resolve the issue while maintaining the flexibility of the
RUN_ASenvironment variable. The implementation is clean, efficient, and aligns well with Docker best practices.
See conversation here. Tested locally; it works! cc @burnettk
Summary by CodeRabbit