Extract shared discover plugin logic into the step

[therazix]

This PR extracts some shared behavior (repository fetching, installation of libraries, and application of policies) from individual discover plugins to their parent Discover plugin. The discover step now has control over these individual actions and can, for example, choose when exactly to perform repository fetching and other actions. It also makes some behavior consistent between the two discover plugins. The discover/shell plugin now gains the url-content-type key, which works the same way as in the discover/fmf plugin. Remote repository fetching is now unified for both plugins.

Resolves #4348

Pull Request Checklist

• implement the feature
• write the documentation
• extend the test coverage
• update the specification
• adjust plugin docstring
• modify the json schema
• mention the version
• include a release note

[thrix]

/packit retest-failed

[thrix]

@therazix looks good, I just found one thing via Claude Code Opus 4.5 to review. Can you check on it please?

Bug: Incorrect keep-git-metadata validation when using remote URL in discover/shell

Summary

The refactoring introduced a regression where the keep-git-metadata validation incorrectly runs when cloning a remote URL. The validation checks the local repository structure (git_root vs fmf_root), which is irrelevant when cloning a remote repository.

Location

tmt/steps/discover/shell.py:331-341

Original Behavior (main branch)

The validation only ran when no URL was provided (local repository case):

def go(self, ...):
    if self.data.url:
        # Clone remote repo - NO validation about local git structure
        self.fetch_remote_repository(self.data.url, self.data.ref, testdir, keep_git_metadata)
    else:
        # Local repo only:
        if keep_git_metadata:
            if git_root != tree_root:
                raise DiscoverError("The 'keep-git-metadata' option can be "
                                    "used only when fmf root is the same as git root.")
            self.run(Command("rsync", ...))

New Behavior (this PR)

The validation now runs regardless of whether a URL is provided:

def go(self, ...):
    # Runs ALWAYS, even when URL is used
    if keep_git_metadata:
        if self.step.plan.fmf_root:
            git_root = tmt.utils.git.git_root(
                fmf_root=self.step.plan.fmf_root, logger=self._logger
            )
            if git_root and git_root != self.step.plan.fmf_root:
                raise DiscoverError(...)  # BUG: also triggers when URL is used!
    else:
        shutil.rmtree(self.test_dir / '.git', ignore_errors=True)

Reproduction Scenario

Local repo structure:
  /home/user/myrepo/           <- git root
  /home/user/myrepo/tests/     <- fmf root (different from git root)

Plan at /home/user/myrepo/tests/plan.fmf:

discover:
    how: shell
    url: https://github.com/teemtee/fmf
    keep-git-metadata: true
    tests:
    - name: test
      test: test -e .git

Expected: Clone remote repo successfully, keep its .git directory

Actual: Error: The 'keep-git-metadata' option can be used only when fmf root is the same as git root.

The error incorrectly refers to the local repo structure, which should be irrelevant when cloning a remote URL.

Why CI Passes

The existing test /url/keep runs git init at the data directory level, making git_root == fmf_root, so the spurious validation doesn't trigger.

Suggested Fix

Remove the duplicated/incorrect validation from go() since _fetch_local_repository() already handles the local repo case correctly:

def go(self, ...):
    keep_git_metadata = True if self.data.dist_git_source else self.data.keep_git_metadata

    if not keep_git_metadata:
        # Remove .git so that it's not copied to the SUT
        shutil.rmtree(self.test_dir / '.git', ignore_errors=True)
    # Remove the validation block - already handled in _fetch_local_repository()

Or, if the validation must remain in go() for some reason, guard it with a URL check:

if keep_git_metadata and not self.data.url:
    # Validation only applies to local repos
    if self.step.plan.fmf_root:
        git_root = tmt.utils.git.git_root(...)
        if git_root and git_root != self.step.plan.fmf_root:
            raise DiscoverError(...)
else:
    if not keep_git_metadata:
        shutil.rmtree(self.test_dir / '.git', ignore_errors=True)

[therazix]

@therazix looks good, I just found one thing via Claude Code Opus 4.5 to review. Can you check on it please?

Nice catch, thanks! I completely missed the duplicate validation. It should be fixed in 562eaa0.

[thrix]

@therazix looks good, I just found one thing via Claude Code Opus 4.5 to review. Can you check on it please?

Nice catch, thanks! I completely missed the duplicate validation. It should be fixed in 562eaa0.

Ty, can we also add tests?

  1. Add test plan in tests/discover/libraries/data/plan.fmf

  /shell:
      summary: "Library installation with shell discover plugin"
      discover:
          how: shell
          tests:
            - name: /certificate-shell
              test: ./certificate.sh
              require:
                - library(openssl/certgen)

  2. Add test phase in tests/discover/libraries/test.sh

  Add this phase before the cleanup:

  rlPhaseStartTest "Shell discover plugin installs libraries"
      rlRun -s "$tmt shell"
      rlAssertGrep "Fetch library 'openssl/certgen'" $rlRun_LOG
  rlPhaseEnd

[thrix]

LGTM after adding tests.

[LecrisUT]

Ty, can we also add tests?

  1. Add test plan in tests/discover/libraries/data/plan.fmf

  /shell:
      summary: "Library installation with shell discover plugin"
      discover:
          how: shell
          tests:
            - name: /certificate-shell
              test: ./certificate.sh
              require:
                - library(openssl/certgen)

Would prefer to keep it similar to file.sh that is already there. It seems that that would require changes on #4427 also because I had an assumption in there which I should be able to relax, but we would need more coverage in /tests/libarires

[LecrisUT]

@therazix do you have a tracker for unifying the rest of these options? I don't see why keep_git_metadata should be only in shell or why the dist_git_* are only in fmf. There is still a lot of other discrepancy that we need to deal with, but just tracking these discrepancies would be helpful.

[thrix]

@therazix do you have a tracker for unifying the rest of these options? I don't see why keep_git_metadata should be only in shell or why the dist_git_* are only in fmf. There is still a lot of other discrepancy that we need to deal with, but just tracking these discrepancies would be helpful.

In that case I would keep it for the next release with an issue