Overall, the structure of this looks good.

In check_repo()

if not os.path.exists(src_repo_path)

maybe os.path.isdir?

json.load(open(f'{src_repo_path}/repo.json'))

Elsewhere we load json files a little differently. Should we use koji.load_json() here, or otherwise involve _open_text_file?

In check_arch_repo()

if src_exists != os.path.exists(dst_file): 

We should probably decline to copy (and perhaps warn) if pkglist/blocklist is missing for either, as repo_init should always write them. Or am I missing some edge case?

in copy_repo()

Perhaps name it copy_arch_repo() for precision and consistency?

#dst_repo_path = koji.pathinfo.repo(repo_id, taginfo['name'])
...
#dst_repodata = f'{dst_repo_path}/{arch}/repodata'

debugging lines? still relevant?

shutil.copytree(src_repodata, dst_repodata)

This had me checking whether we need symlinks=True (we don't, because the toplink symlink is one dir up), but perhaps worth a comment?

with open(f'{dst_repodata}/repo.json', 'wt') as fp:
    json.dump({'cloned_from_repo_id': src_repo_id}, fp, indent=2)

This seems to overwrite the json with just that single bit of data

self.logger.warning(f"Copying repo {src_repo_id} to {repo_id} failed. {ex}")
return False

Returning False here will trigger a createrepo run, but the failure may have left partial repodata copied. We should probably clean it up (and actually fail if we cannot).

in handler()

self.logger.debug("DEBUG: %r : %r " % (arch, data[arch],))

we're outside of the arch loop here. needs adjustment

We're not checking for maven support here. That is something I listed in #3841, but perhaps it is ok in this context? I guess the resulting createrepo job isn't going to look at the maven repo and doesn't care about it. Regardless, worth a comment to explain our thinking.

repo_json = koji.load_json(f'{src_repodata}/repo.json')
repo_json['cloned_from_repo_id'] = src_repo_id
koji.dump_json(f'{dst_repodata}/repo.json', repo_json, indent=2

This code from copy_arch_repo is using the wrong location for repo.json. This file lives in the main repo directory, and is not stored per arch. So this bit doesn't fit in this function.

This exposes a larger issue -- we don't have a mechanism for the builder to update repo.json, so we'll need to add one. Perhaps this can be added to the repoDone call.

Hmm, perhaps I was mistaken in my previous review. Did you intend to add a new per-arch repo.json file that only includes the cloned repo id? I do like the idea of tracking the clone links, but I'm not sure about the location. Having multiple repo.json files could be confusing. Maybe I'm being pedantic?

Yes, idea was to have these arch specific files. Anyway, you're right that it means too many files with same name.
So, maybe adding separate record to repo-level repo.json?

{
  "id": 562,
  "tag": "a",
  "tag_id": 19,
  "task_id": 1673,
  "event_id": 2843,
  "with_src": false,
  "with_separate_src": false,
  "with_debuginfo": false,
  "cloned_from_repo_id": {
      "x86_64": 123,
      "aarch64": 123,
  }
} 

Other archs are not present or cloned?

second option:

{
   ...
  "cloned_from_repo_id":  123,
  "cloned_archs": ["x86_64", "aarch64"],
}

The current path updates are only affecting how things are written out to the workdir on the builder. copy_arch_repo only returns the list of files in dst_repodata, and this result ends up getting passed through to the host.repoDone call. Nothing in this current code seems to do anything with the updated repo.json file.

As I mentioned above, if we want to update repo.json (which previously was only written on the hub by repoInit), we'll need to add a mechanism for that. Perhaps a new option to repoDone. I guess we could also cram it into the data arg, but that could be obtuse and confusing.

Second option is probably better. We should always have a single repo id we're cloning from. I don't think we want to enable copying different repos per arch.

:thumbsup:

Commit c3c0a6a1 fixes this pull-request