Return error message when no fmf root found in ansible prepare

[falconizmi]

Pull Request Checklist

• implement the feature

[happz]

Seems needlessly complicated :)

if not self.step.plan.fmf_root:
    raise PrepareError("No fmf root found. Directory is not initialized with `tmt init`")

[falconizmi]

I already tried to do it like that but its not working, because fmf_root is not value but a getter, its failing before it can go to inside the if statement.

[happz]

I see. But that's very interesting information: it fails because the getter, declared as -> Path, runs return Path(self.node.root), and we realized an fmf node may not always be present. So what you do is scratching the surface, your change will turn "give me fmf root" into an exception in one place only, while everything else trying to access fmf root will be left unsanitized. We should go deeper :)

So, it feels like this check & exception should be raised by fmf_root itself, to prevent anyone from accessing fmf_root without a punishment. I'm not sure how exactly the exception raised now by getter looks like, could you share it? Or feel free to design the test right away.

BTW there's enough blame for more pieces of code, e.g. all these classes, Core, Test, Plan, Story - they take fmf node as an input parameter: how can they take an fmf node when we agreed that without an fmf node, there is no fmf tree and therefore no fmf nodes representing plans and tests?? How can we ever get to run a playbook, as directed by a prepare step of a plan when there is no fmf root and therefore no plans to recognize?? So there is more to review, basically, all constructs based on fmf are now suspicious when they can get constructed without an fmf root and get as far as to run a playbook :) I would start by checking whether we could put the check & exception into fmf_root for now, but eventually, we might check how can we even have Plan instance running playbooks when we have no fmf root to start with, and push this check to trigger way sooner.

[falconizmi]

Is the new change enough for now? I tested it and it returns me the exception message
~/.local/bin/tmt run -vvv prepare --how ansible --playbook packages.yml provision --how virtual (in directory without tmt init)

[lukaszachy]

, we might check how can we even have Plan instance running playbooks when we have no fmf root to start with, and push this check to trigger way sooner.

But we (technically) do allow to specify the plan via CLI, right? IMO in that case fmf root doesn't need to exist.

[psss]

Maybe we should have something like self.anchor_path that would provide this particular path

Yeah, that sounds good.

[falconizmi]

If I understand correctly, instead of having property fmt_root and its getter in base.py, we will have anchor_path property and its getter (that would try to return fmf_root and if it fails, it would return Path.cwd() ) and I would change only those which used fmf_root getter from base.py and the rest of fmf_root usage we would keep them as they are?

[happz]

I wouldn't say instead, sometimes it might be good to know what's fmf root. I'd say it's rather an addition, anchor_path would perform this self.fmf_root or Path.cwd() dance, and it should be used instead of fmf_root at places where fmf_root is used today, yes.

[falconizmi]

So in fmf_root getter instead of returning exception, it would return None

[happz]

Yes, that's what I would do. If objects like Plan can exist without an fmf root, then it shouldn't be a sin to access Plan.fmf_root, instead an "undefined" value like None should be returned.