Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new Artemis test migration inventory host and host_vars (including Vault-sourced credentials and container/image names) and three Ansible playbooks to set up, reset, and load a migrated database on that host. Changes
Sequence Diagram(s)sequenceDiagram
participant Ansible as "Ansible Controller"
participant Vault as "HashiCorp Vault"
participant Host as "artemis-test-migration"
participant Docker as "Docker / Containers"
participant MySQL as "MySQL Container"
Ansible->>Vault: request DB & admin secrets
Vault-->>Ansible: return credentials
Ansible->>Host: run playbook (migration_setup / reset / load)
Host->>Docker: start/configure containers (artemis-db, artemis-temp-db)
Docker->>MySQL: provision image `mysql:8.0.45` and apply dump (if load)
MySQL-->>Host: report status
Host-->>Ansible: playbook success/failure
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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)
📝 Coding Plan
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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
playbooks/artemis-migration/migration_load_new_db_backup.yml (1)
3-3: Prefer targeting the inventory group instead of a hardcoded FQDN.Line 3 hardcodes a single host; using
artemistestmigrationreduces duplication and avoids edits across multiple playbooks if the hostname changes.Proposed change
- hosts: artemis-test-migration.artemis.cit.tum.de + hosts: artemistestmigration🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playbooks/artemis-migration/migration_load_new_db_backup.yml` at line 3, The playbook currently hardcodes a single FQDN in the hosts directive ("hosts: artemis-test-migration.artemis.cit.tum.de"); update the hosts line in migration_load_new_db_backup.yml to target the inventory group name (e.g., "hosts: artemisestmigration" or the canonical group you maintain) so the playbook uses the inventory group instead of a fixed host, ensuring other tasks (and any includes/roles) implicitly target that group.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@playbooks/artemis-migration/migration_load_new_db_backup.yml`:
- Around line 7-10: The playbook sets become: true but uses a tilde in the
dump_src var which will expand to root's home at runtime; update the dump_src
variable (the dump_src YAML key in migration_load_new_db_backup.yml) to an
explicit absolute path to the backup (e.g. /home/youruser/artemis_backup.sql.gz
or wherever the file actually lives) so the task that reads dump_src finds the
correct file when run with privilege escalation.
---
Nitpick comments:
In `@playbooks/artemis-migration/migration_load_new_db_backup.yml`:
- Line 3: The playbook currently hardcodes a single FQDN in the hosts directive
("hosts: artemis-test-migration.artemis.cit.tum.de"); update the hosts line in
migration_load_new_db_backup.yml to target the inventory group name (e.g.,
"hosts: artemisestmigration" or the canonical group you maintain) so the
playbook uses the inventory group instead of a fixed host, ensuring other tasks
(and any includes/roles) implicitly target that group.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
host_vars/artemis-test-migration.artemis.cit.tum.de.ymlhostsplaybooks/artemis-migration/migration_load_new_db_backup.ymlplaybooks/artemis-migration/migration_reset_db.ymlplaybooks/artemis-migration/migration_setup.yml
| become: true | ||
| vars: | ||
| dump_src: "~/artemis_backup.sql.gz" | ||
| bake: true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n playbooks/artemis-migration/migration_load_new_db_backup.yml | head -20Repository: ls1intum/artemis-ansible
Length of output: 367
🏁 Script executed:
# Check if the file exists and its full content around the specified lines
wc -l playbooks/artemis-migration/migration_load_new_db_backup.ymlRepository: ls1intum/artemis-ansible
Length of output: 131
🏁 Script executed:
# Search for any other usages of dump_src or similar paths in the playbook
rg "dump_src|become" playbooks/artemis-migration/migration_load_new_db_backup.yml -A 2 -B 2Repository: ls1intum/artemis-ansible
Length of output: 216
🏁 Script executed:
# Look for the migration_tester role to understand how dump_src is used
find . -path "*/migration_tester*" -type dRepository: ls1intum/artemis-ansible
Length of output: 50
🏁 Script executed:
# Search for other uses of dump_src to understand the pattern
rg "dump_src" -B 3 -A 3Repository: ls1intum/artemis-ansible
Length of output: 512
🌐 Web query:
Ansible tilde home directory expansion with become privilege escalation
💡 Result:
~ expansion in Ansible is frequently confusing with become because:
- Most Ansible modules do not perform shell-style tilde expansion. For example,
copy.destexpects a remote path (typically absolute); if you pass~, Ansible won’t reliably expand it the way an interactive shell would. (docs.ansible.com) - When tilde expansion does happen (typically only via a shell on the target, e.g.,
ansible.builtin.shell), it expands to the home of the effective user. Withbecome: true, the effective user is usuallyroot, so~becomes/root, not the SSH login user’s home. (docs.ansible.com) become_useronly specifies who you become; it doesn’t implybecome: true. (docs.ansible.com)
Practical fixes / patterns
-
Avoid
~in module paths; use an explicit absolute path.
Example (install a user’s authorized keys while escalating to root):- copy: src: files/authorized_keys dest: "/home/{{ target_user }}/.ssh/authorized_keys" owner: "{{ target_user }}" group: "{{ target_user }}" mode: "0600" become: true
-
If you truly need “that user’s home”, look it up (don’t guess).
Common approach: query passwd DB, then build the path (works even if homes aren’t/home/<user>). -
If you rely on shell expansion, remember it’s user-dependent under
become.
~expands based on the user running the shell (root underbecome). This is standard shell behavior. (stackoverflow.com) -
Controller-side
~expansion is different from remote.
Ansible has anexpanduserfilter, but it expands~where templating runs (the controller), so it’s usually not what you want for remote home directories. (docs.ansible.com)
Citations:
- 1: https://docs.ansible.com/projects/ansible/latest/collections/ansible/builtin/copy_module.html?utm_source=openai
- 2: https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_privilege_escalation.html?utm_source=openai
- 3: https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_privilege_escalation.html?utm_source=openai
- 4: https://stackoverflow.com/questions/5748216/tilde-in-path-doesnt-expand-to-home-directory?utm_source=openai
- 5: https://docs.ansible.com/ansible/latest/collections/ansible/builtin/expanduser_filter.html?utm_source=openai
🏁 Script executed:
rg "become_user|migration_tester" playbooks/artemis-migration/ -B 2 -A 2Repository: ls1intum/artemis-ansible
Length of output: 741
🏁 Script executed:
# Check if there are other playbooks with similar patterns to understand context
rg "become.*true" playbooks/ -l | head -5Repository: ls1intum/artemis-ansible
Length of output: 294
Use an absolute dump_src path when running with become.
Line 9 uses ~, but line 7 enables privilege escalation. Ansible does not perform shell-style tilde expansion in module arguments, and when expansion occurs via shell (e.g., with shell modules), ~ resolves to the effective user's home. With become: true and no become_user specified, the effective user is root, so ~ becomes /root, not the original user's home—causing the dump file to not be found at runtime.
Use an explicit absolute path instead:
Safer path example
- dump_src: "~/artemis_backup.sql.gz"
+ dump_src: "/tmp/artemis_backup.sql.gz"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playbooks/artemis-migration/migration_load_new_db_backup.yml` around lines 7
- 10, The playbook sets become: true but uses a tilde in the dump_src var which
will expand to root's home at runtime; update the dump_src variable (the
dump_src YAML key in migration_load_new_db_backup.yml) to an explicit absolute
path to the backup (e.g. /home/youruser/artemis_backup.sql.gz or wherever the
file actually lives) so the task that reads dump_src finds the correct file when
run with privilege escalation.
| @@ -0,0 +1,12 @@ | |||
| --- | |||
There was a problem hiding this comment.
Isn't this already covered by the playbook in playbooks/artemis-tests/artemis-tests.yml?
There was a problem hiding this comment.
It is. You'd just have to limit the hosts to the migration tester. if you'd like me to remove the file, I will do so
It needs github.com/ls1intum/artemis-ansible-collection/pull/190 to be merged for it to work |
Mtze
left a comment
Mtze
left a comment
There was a problem hiding this comment.
True - i reviewed the other one 👍
3 participants
Motivation and Context
This are the files to use artemis-test-migration; for more, see ls1intum/artemis-ansible-collection#190
Description
Summary by CodeRabbit
New Features
Chores