users are not able to choose workdir-root if they run tmt clean command

[skycastlelily]

As def run doesn't accept a @workdir_root_options and workdir_root is hard coded to context.params['workdir_root'] = tmt.utils.WORKDIR_ROOT:https://github.com/teemtee/tmt/blob/main/tmt/cli.py#L1784, gonna to send a merge request to add the support

[happz]

Seems to be more complicated:

• clean subcommands do accept --workdir-path, and use it, see https://github.com/teemtee/tmt/blob/main/tmt/cli.py#L1861
• clean subcommands seem to not use effective_workdir_root, https://github.com/teemtee/tmt/blob/main/tmt/utils.py#L254, i.e. they don't obey a TMT_WORKDIR_ROOT envvar
• clean command should definitely use effective_workdir_root instead of tmt.utils.WORKDIR_ROOT
• effective_workdir_root has no input for CLI option

I'd propose to:

• update effective_workdir_root to accept one optional parameter, the value of CLI option. The CLI option should not have a default set - it should deliver None when not set, and effective_workdir_root should deliver the default value unless overridden. Something like this:

  def effective_workdir_root(workdir_root_option: Optional[str] = None) -> Path:
      if workdir_root_option:
          return Path(workdir_root_option)
  
      if 'TMT_WORKDIR_ROOT' in os.environ:
          return Path(os.environ['TMT_WORKDIR_ROOT'])
  
      return WORKDIR_ROOT

• then we can switch all users of tmt.utils.WORKDIR_ROOT, like that clean command, to use effective_workdir_root. Code with access to CLI options - clean, clean runs, etc. - would add in workdir_root value, the rest wouldn't, everyone's happy and using one way to infer a workdir root.

• clean itself probably does not have to have its own --workdir-root option - context.params['workdir_root'] = tmt.utils.effective_workdir_root(workdir_root_option=context.params.get('workdir_root') ) should do the trick, use option passed to the subcommand from clean command code.

Edit: clean does deserve its own option, as it can be run on its own. Needs to go through effective_workdir_root anyway, cannot use workdir_root parameter directly.

[skycastlelily]

Ah, your approach is much better than mine: I was thinking removed the hardcode line and change if self.tmt.utils.WORKDIR_ROOT.exists() -> if Path(workdir_root).exists() or self.tmt.utils.WORKDIR_ROOT.exists()

clean itself probably does not have to have its own --workdir-root option

- context.params['workdir_root'] = tmt.utils.effective_workdir_root(workdir_root_option=context.params.get('workdir_root') ) should do the trick, use option passed to the subcommand from clean command code Np, clean itself need its own one, users may run "tmt clean" to clean everything.

effective_workdir_root has no input for CLI option

yep, we may also need add @workdir_root_options to def run,but that's not related to this merge request^^

…

On Tue, Apr 2, 2024 at 10:53 PM Miloš Prchlík ***@***.***> wrote: Seems to be more complicated: - clean subcommands do accept --workdir-path, and use it, see https://github.com/teemtee/tmt/blob/main/tmt/cli.py#L1861 - clean subcommands seem to not use effective_workdir_root, https://github.com/teemtee/tmt/blob/main/tmt/utils.py#L254, i.e. they don't obey a TMT_WORKDIR_ROOT envvar - clean command should definitely use effective_workdir_root instead of tmt.utils.WORKDIR_ROOT - effective_workdir_root has no input for CLI option I'd propose to: - update effective_workdir_root to accept one optional parameter, the value of CLI option. The CLI option should not have a default set - it should deliver None when not set, and effective_workdir_root should deliver the default value unless overridden. Something like this: def effective_workdir_root(workdir_root_option: Optional[str] = None) -> Path: if workdir_root_option: return Path(workdir_root_option) if 'TMT_WORKDIR_ROOT' in os.environ: return Path(os.environ['TMT_WORKDIR_ROOT']) return WORKDIR_ROOT - then we can switch all users of tmt.utils.WORKDIR_ROOT, like that clean command, to use effective_workdir_root. Code with access to CLI options - clean, clean runs, etc. - would add in workdir_root value, the rest wouldn't, everyone's happy and using one way to infer a workdir root. - clean itself probably does not have to have its own --workdir-root option - context.params['workdir_root'] = tmt.utils.effective_workdir_root(workdir_root_option=context.params.get('workdir_root') ) should do the trick, use option passed to the subcommand from clean command code. — Reply to this email directly, view it on GitHub <#2807 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKFR23CBKEW4NV7QMP5HQ5DY3LA5VAVCNFSM6AAAAABFTQKD4KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZSGI3DSMBTHE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[psss]

Seems the approach is outlined, @skycastlelily would you be willing to work on this for 1.36.

[skycastlelily]

Sure,my pleasure:)

…

On Fri, Aug 9, 2024 at 7:26 PM Petr Šplíchal ***@***.***> wrote: Seems the approach is outlined, @skycastlelily <https://github.com/skycastlelily> would you be willing to work on this for 1.36. — Reply to this email directly, view it on GitHub <#2807 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKFR23HZQQBJUZGSSGWJCNDZQSRP3AVCNFSM6AAAAABFTQKD4KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZXG4ZTKMJRG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[skycastlelily]

Here is the mr for this issue: #2850 :)