Conversation
📝 WalkthroughWalkthroughAdds a new Ansible play targeting Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Dev / CI
participant Ansible as Ansible Controller
participant Vault as HashiCorp Vault
participant Hermes as Hermes Host
rect rgba(240,248,255,0.5)
Dev->>Ansible: run playbook playbooks/hermes/hermes.yml
end
rect rgba(255,250,240,0.5)
Ansible->>Vault: lookup APNS cert path, cert_password, apns_cert_content
Ansible->>Vault: lookup google_application_credentials path, content
Vault-->>Ansible: return secrets
end
rect rgba(240,255,240,0.5)
Ansible->>Hermes: apply role ls1intum.artemis.hermes (setup_system: true) with vars
Hermes-->>Ansible: acknowledge / configure service, write certs, set port 17333
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
playbooks/hermes/hermes.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: otoriphoenix
Repo: ls1intum/artemis-ansible PR: 33
File: group_vars/artemistests_common_config.yml:60-69
Timestamp: 2025-10-13T13:13:14.474Z
Learning: The artemis-ansible repository (ls1intum/artemis-ansible) provides configuration values for the Artemis Ansible Collection (ls1intum/artemis-ansible-collection). The values in files like group_vars/artemistests_common_config.yml are concrete settings consumed by the collection, not variables requiring defaults.
🪛 GitHub Check: 🕵️♀️ Ansible Lint
playbooks/hermes/hermes.yml
[failure] 6-6: syntax-check[specific]
the role 'ls1intum.artemis.hermes' was not found in /home/runner/work/artemis-ansible/artemis-ansible/playbooks/hermes/roles:/home/runner/work/artemis-ansible/artemis-ansible/.ansible/roles:/home/runner/work/artemis-ansible/artemis-ansible/roles:/home/runner/work/artemis-ansible/artemis-ansible/roles:/home/runner/work/artemis-ansible/artemis-ansible/roles/community:/home/runner/work/artemis-ansible/artemis-ansible/playbooks/hermes
🔇 Additional comments (3)
playbooks/hermes/hermes.yml (3)
10-10: Clarify path resolution for certificate and credentials files.The relative paths
key/artemis-ios-apns-prod.p12andkey/artemis.jsonlack context about where they are resolved from. Confirm whether:
- These paths are resolved relative to the
hermes_working_directory(/opt/hermes)?- Are they resolved relative to the playbook directory?
- Do they need to be absolute paths instead?
Document the expected path resolution or update to absolute paths if needed for clarity.
Also applies to: 14-14
11-12: Verify Vault secret keys match the stored structure.The playbook retrieves three secrets from Vault path
kv/data/artemis/production/hermes:
apns_certificate_password(line 11)apns_certificate_base64(line 12)google_certificate(line 15)Confirm these key names match exactly what is stored in Vault. A key mismatch will cause
.get()to returnNone, leading to configuration failures. No documentation or test configuration in the codebase references this Vault path or its expected structure, so manual verification against Vault or infrastructure documentation is required.
6-6: The original review comment is incorrect.The
ls1intum.artemiscollection is properly declared in requirements.yml, and the collection includes the Hermes role. The playbook uses the correct namespaced reference format (ls1intum.artemis.hermes). There is no "role not found" issue in the codebase—the role will be resolvable when the collection is installed as specified in the dependencies. The original ansible-playbook command failure was due to the sandbox environment lacking Ansible tools, not a code problem.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
hosts (1)
129-131: Consider adding a section header for consistency.Other major sections in this file have descriptive comment headers (e.g., lines 1-3 for Artemis Tests, lines 132-134 for Artemis Staging LocalCI). Adding a similar header for the Hermes section would improve maintainability.
+############################### +# Hermes +############################### [hermes] hermes.artemis.cit.tum.de
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hosts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-13T13:13:14.474Z
Learnt from: otoriphoenix
Repo: ls1intum/artemis-ansible PR: 33
File: group_vars/artemistests_common_config.yml:60-69
Timestamp: 2025-10-13T13:13:14.474Z
Learning: The artemis-ansible repository (ls1intum/artemis-ansible) provides configuration values for the Artemis Ansible Collection (ls1intum/artemis-ansible-collection). The values in files like group_vars/artemistests_common_config.yml are concrete settings consumed by the collection, not variables requiring defaults.
Applied to files:
hosts
🔇 Additional comments (1)
hosts (1)
129-131: Code configuration verified; DNS and Vault backend setup remains operational concern.The hermes host group addition is properly configured. Verification confirms:
- Playbook
playbooks/hermes/hermes.ymlexists and correctly references thels1intum.artemis.hermesrole- All required vault secrets are properly configured with
lookup('hashi_vault', 'kv/data/artemis/production/hermes')for APNS and Google credentials- Service configuration (port 17333, working directory, certificate paths) is complete
The infrastructure team should verify DNS resolution of
hermes.artemis.cit.tum.deand confirm vault backend secrets are populated, but the code changes are ready.
|
Already looking good :) Would you mind adding the test env as well? And there seems to be a linter issue |
|
Apparently, the lint issue is that ls1intum/artemis-ansible-collection#170 has not been merged yet. |
Mtze
left a comment
There was a problem hiding this comment.
I think we need to bump the version in the requirements.yml for the runnter to see the new role + inline
Summary by CodeRabbit