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 https://github.com/teemtee/tmt/issues/2807#issuecomment-2032269039, 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 https://github.com/teemtee/tmt/issues/2807#issuecomment-2277735117, 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: https://github.com/teemtee/tmt/pull/2850 :)

[psss]

This seems to be already fixed in #2850, closing.