Expose obsah_state_path variable#86
Conversation
|
CI is unhappy, as it asserts the set variables, and you changed that. But otherwise LGTM |
stejskalleos
force-pushed
the
ls/obsah_state_dir
branch
from
2b2b788 to
52d3adb
Compare
evgeni
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
🍏 |
WalkthroughThe change introduces a new extra-vars entry obsah_state_path in generate_ansible_args within obsah/init.py, setting it from args.playbook.application_config.state_path(). This value is serialized into the Ansible command line JSON variables. Corresponding tests in tests/test_obsah.py are updated to expect an added -e '{"obsah_state_path": "/var/lib/obsah"}' across various CLI scenarios. No public interfaces or exported declarations are altered. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (2)
obsah/__init__.py (1)
395-406: Confirm precedence: generated JSON -e overrides user-supplied -e.Because extra_vars are appended first (Lines 397-399) and the JSON-serialized variables are appended after (Lines 405-406), any obsah_state_path provided via -e (key=value/JSON/@file) will be overridden by the generated value. This mirrors the current policy that CLI-exposed vars beat -e, but it’s worth double-checking that this is intentional for obsah_state_path too.
If you prefer user -e to be able to override, invert the order of assembling -e arguments:
- for extra_var in getattr(args, 'extra_vars', []): - ansible_args.extend(["-e", extra_var]) - ... - if variables: - ansible_args.extend(["-e", json.dumps(variables, sort_keys=True)]) + if variables: + ansible_args.extend(["-e", json.dumps(variables, sort_keys=True)]) + for extra_var in getattr(args, 'extra_vars', []): + ansible_args.extend(["-e", extra_var])If the current precedence is desired, consider adding a short comment noting that the generated -e is appended last to intentionally win over user-supplied -e for consistent behavior.
tests/test_obsah.py (1)
93-118: Tests updated appropriately; consider hardening against env/formatting and asserting precedence.Nice coverage of all CLI permutations with the added obsah_state_path expectation. To reduce flakiness and make intent explicit:
- Control OBSAH_STATE in tests to avoid environment leakage (CI or dev shells setting OBSAH_STATE would break expectations).
- Prefer asserting the parsed JSON content over exact string formatting to avoid brittleness if separators/sort options change.
- Add a focused test that validates the intended precedence between user -e and the generated obsah_state_path.
Example additions (outside this hunk):
# Add at module scope to stabilize environment for all tests import os, json import pytest @pytest.fixture(autouse=True) def _ensure_default_state_path(monkeypatch): # Ensure tests don't inherit an external OBSAH_STATE monkeypatch.delenv('OBSAH_STATE', raising=False) def test_state_path_from_env_overrides_default(playbooks_path, parser, monkeypatch): monkeypatch.setenv('OBSAH_STATE', '/tmp/obsah') args = parser.parse_args(['setup']) ansible_args = obsah.generate_ansible_args('inventory.yml', args, parser.obsah_arguments) assert ansible_args[-2] == '-e' payload = json.loads(ansible_args[-1]) assert payload['obsah_state_path'] == '/tmp/obsah' def test_obsah_state_path_precedence_over_user_extra_vars(playbooks_path, parser): # User tries to override via -e, generated JSON should win with current ordering args = parser.parse_args(['setup', '-e', 'obsah_state_path=/should/not/win']) ansible_args = obsah.generate_ansible_args('inventory.yml', args, parser.obsah_arguments) # The last -e is the generated JSON; verify its value prevails assert ansible_args[-2] == '-e' payload = json.loads(ansible_args[-1]) assert payload['obsah_state_path'] == '/var/lib/obsah'If you decide to allow user -e to override (by reordering as suggested in init.py), flip the last assertion accordingly and add a complementary test for JSON-style -e too.
I can wire these tests into the suite and adjust expectations if you choose the other precedence policy. Want me to push an updated test patch?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
obsah/__init__.py(1 hunks)tests/test_obsah.py(1 hunks)
🔇 Additional comments (1)
obsah/__init__.py (1)
401-404: LGTM: exposing obsah_state_path is clear and consistent.Using variables['obsah_state_path'] = args.playbook.application_config.state_path() reads cleanly, matches the existing ApplicationConfig API, and aligns with the prior naming feedback. Tests appear updated accordingly.
2 participants
No description provided.