Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Save remote ansible playbooks to correct directory #3527

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

therazix
Copy link
Collaborator

@therazix therazix commented Feb 17, 2025

This PR changes the location where remote ansible playbooks are saved. Previously, they were saved in the plan's directory, but now they are saved in the workdir (/var/tmp/tmt/...) to ensure that tmt does not modify the plan directory.

This change also resolves the issue with remote ansible playbooks not functioning correctly when used with imported plans.

Fixes: #3237
Fixes: #3349

Pull Request Checklist

  • implement the feature
  • write the documentation
  • extend the test coverage

@therazix therazix added step | prepare Stuff related to the prepare step plugin | ansible The ansible plugins labels Feb 17, 2025
@happz happz added the ci | full test Pull request is ready for the full test execution label Feb 17, 2025
@happz
Copy link
Collaborator

happz commented Feb 17, 2025

/packit build

@therazix therazix force-pushed the fvagner-remote-playbook branch from bf08f24 to 8855a08 Compare February 18, 2025 14:47
@therazix therazix changed the title Fix remote ansible playbooks in imported plans Save remote ansible playbooks to correct directory Feb 18, 2025
@therazix therazix added ci | full test Pull request is ready for the full test execution and removed ci | full test Pull request is ready for the full test execution labels Feb 18, 2025
@therazix
Copy link
Collaborator Author

@happz I noticed there is a similar issue #3237 and I tried to fix it in 8855a08. Now, remote playbooks are saved in the prepare directory inside the workdir (/var/tmp/tmt/run-xyz/plans/plan_name/prepare), which gets purged during the finish step. Does this approach make sense, or would it be better to use a different directory so the playbook is preserved beyond the finish step?

@therazix therazix requested a review from happz February 18, 2025 15:08
@happz
Copy link
Collaborator

happz commented Feb 23, 2025

@happz I noticed there is a similar issue #3237 and I tried to fix it in 8855a08.

Makes sense, it seems to be the same kind of trouble.

Now, remote playbooks are saved in the prepare directory inside the workdir (/var/tmp/tmt/run-xyz/plans/plan_name/prepare), which gets purged during the finish step. Does this approach make sense, or would it be better to use a different directory so the playbook is preserved beyond the finish step?

I'd say workdir is a good option. We can always improve finish to not remove them when pruning its workdir, either by giving them some shared filename prefix (ansible-remote-playbook-foo.yaml?) or putting remote playbooks in a dedicated directory ($workdir/remote-ansible-playbooks?).


if lowercased_playbook.startswith(('http://', 'https://')):
assert self.step.workdir is not None # narrow type
playbook_root = self.step.workdir
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As soon as self.step.workdir appears for the second time, I'd suggest changing the normalize_* family of functions to return both playbook root and playbook, i.e. -> tuple[Path, AnsibleApplicable], and most of them would return anchor_path but normalize_remote_playbook would return (self.step.workdir, Path(file.name).relative_to(root_path)).

That way, the "this kind of playbook lives in step workdir" knowledge would be owned by the code responsible for processing the raw playbook path, instead of being "shared" by this code and code calling it.

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea. Updated in 590b020.

@therazix therazix force-pushed the fvagner-remote-playbook branch from 8855a08 to 590b020 Compare February 24, 2025 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci | full test Pull request is ready for the full test execution plugin | ansible The ansible plugins step | prepare Stuff related to the prepare step
Projects
None yet
2 participants