🐛 Disable hub chown init container by default and always exit 0#540
Conversation
Signed-off-by: Jason Montleon <jmontleo@redhat.com>
📝 WalkthroughWalkthroughA new Ansible configuration flag Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
roles/tackle/templates/deployment-hub.yml.j2 (1)
242-244: Use/bin/shinstead ofbashfor better portability.The init container uses an external hub image that may not include bash. Line 242 should use
/bin/sh(POSIX standard, available everywhere) instead ofbash(optional in minimal containers). The chown command requires only POSIX features.Suggested diff
- - bash + - /bin/sh - -c - 'chown -R {{ hub_uid }}:root {{ hub_database_volume_path }} {{ hub_bucket_volume_path }} ||:'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@roles/tackle/templates/deployment-hub.yml.j2` around lines 242 - 244, Replace the shell invocation in the init container command to use POSIX /bin/sh instead of bash: update the command entries that currently specify "bash" and the associated "-c" invocation (the init container command that runs 'chown -R {{ hub_uid }}:root {{ hub_database_volume_path }} {{ hub_bucket_volume_path }} ||:') so the first element is "/bin/sh" (keeping "-c" and the chown string unchanged) to ensure the container works in minimal images without bash.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@roles/tackle/templates/deployment-hub.yml.j2`:
- Around line 242-244: Replace the shell invocation in the init container
command to use POSIX /bin/sh instead of bash: update the command entries that
currently specify "bash" and the associated "-c" invocation (the init container
command that runs 'chown -R {{ hub_uid }}:root {{ hub_database_volume_path }} {{
hub_bucket_volume_path }} ||:') so the first element is "/bin/sh" (keeping "-c"
and the chown string unchanged) to ensure the container works in minimal images
without bash.
This init container was originally used to fix permissions when upgrading from an older version that ran as root and probably isn't need most of the time anymore. Allowing it to be enabled still allows for the situation to be corrected if someone is upgrading from a very old version, but otherwise should no longer incur an ohterwise needless wait when the hub is starts. Also added `||:` to ensure the container exits gracefully even if it can't change permissions on read only subdirectories, which may be outside the users control to adjust. This prevents the init container from crashing and preventing the hub from starting. NetApp .snapshot directory is an example of this. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a new configuration option to optionally enable automated file ownership management for database and bucket volumes during container initialization. Disabled by default. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Jason Montleon <jmontleo@redhat.com> Signed-off-by: Cherry Picker <noreply@github.com>
|
PR cherry-picked to branch release-0.9. Backport PR: #541 |
#541) This init container was originally used to fix permissions when upgrading from an older version that ran as root and probably isn't need most of the time anymore. Allowing it to be enabled still allows for the situation to be corrected if someone is upgrading from a very old version, but otherwise should no longer incur an ohterwise needless wait when the hub is starts. Also added `||:` to ensure the container exits gracefully even if it can't change permissions on read only subdirectories, which may be outside the users control to adjust. This prevents the init container from crashing and preventing the hub from starting. NetApp .snapshot directory is an example of this. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a new configuration option to optionally enable automated file ownership management for database and bucket volumes during container initialization. Disabled by default. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Jason Montleon <jmontleo@redhat.com> Signed-off-by: Cherry Picker <noreply@github.com> Signed-off-by: Jason Montleon <jmontleo@redhat.com> Signed-off-by: Cherry Picker <noreply@github.com> Co-authored-by: Jason Montleon <jmontleo@redhat.com>
This init container was originally used to fix permissions when upgrading from an older version that ran as root and probably isn't need most of the time anymore. Allowing it to be enabled still allows for the situation to be corrected if someone is upgrading from a very old version, but otherwise should no longer incur an ohterwise needless wait when the hub is starts.
Also added
||:to ensure the container exits gracefully even if it can't change permissions on read only subdirectories, which may be outside the users control to adjust. This prevents the init container from crashing and preventing the hub from starting. NetApp .snapshot directory is an example of this.Summary by CodeRabbit