Only persist parameters after a successful playbook run

[ehelms]

Parameters were persisted to parameters.yaml before the playbook executed, meaning invalid values (e.g. unknown features) would be written to disk even when the playbook rejected them. On subsequent runs the invalid values were loaded back as defaults, creating a persistent failure loop.

Move persistence to after playbook execution and gate it on a zero exit code so only validated parameters are saved.

If, for example, I run:

./foremanctl deploy --add-feature foo-bar

And foo-bar is not an actual feature, I will get an error message but foo-bar will now be in the features list. This happens consistently if you run the ./forge test command due to https://github.com/theforeman/foremanctl/blob/6e99e94f86956f189a29886591915ae568180901/tests/features_test.py#L24

[arvind4501]

This seems to solve the similar issue #109

[evgeni]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Walkthrough

This PR refactors CLI argument persistence by extracting inline logic from main() into a dedicated persist_args() helper function. The new helper creates the persistence directory if needed, serializes args to YAML, filters out specified keys before writing, and writes to the application config's persist path. main() now calls persist_args() only after successful Ansible playbook execution. Test fixtures are expanded to support the new behavior, and comprehensive tests validate that persist_args() correctly persists filtered arguments and creates nested directories.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• theforeman/obsah#72: Introduces the original CLI-argument persistence mechanism via YAML parameters that this PR refactors into a dedicated helper function.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 5.26% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly describes the main change: moving parameter persistence to after successful playbook execution.
Description check	✅ Passed	The description clearly explains the problem being solved and the solution implemented, directly relating to the changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@obsah/__init__.py`:
- Around line 598-599: The persist_args(...) call can raise I/O exceptions and
currently would make the process exit nonzero even when exit_code == 0; wrap the
call to persist_args(application_config, args, parser.obsah_dont_persist) in a
try/except that catches Exception (or more specific I/O errors like
PermissionError/OSError), log the exception (use the module logger or existing
logger variable) at error level, but do not change or override exit_code so a
successful playbook outcome remains successful; ensure this handling is placed
where application_config.persist_params() and exit_code == 0 are checked.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c3803399-61c5-4a23-a0d5-a1b0ad94a611

📥 Commits

Reviewing files that changed from the base of the PR and between c793d12 and d0fecb0.

📒 Files selected for processing (2)

• obsah/__init__.py
• tests/test_reset.py