Hub:

• why is in __all__ taskrepos and not taskRepoNotifications?
  Testing?

• This is not triggered for scratch builds. postTaskStateChange is correct one.
  Only tasks moved to CLOSED state makes sense here. Not every call. In the
  current code, it is triggered even e.g. if build is moved between volumes.
  Further, it needs to be triggered for every buildArch task.

• repos have to end in /mnt/koji/repos-tasks (s/taskrepos/repos-tasks/
  just for better listing), not in /mnt/work/taskrepos (or maybe in
  /mnt/scratch/repos-tasks - it would be a) easier to automatically
  cleanup, b) more visible that it is just a transient repo) Anyway, thinking
  more about it, /mnt/koji/scratch doesn't look like a good place as it
  would be misleading for non-scratch builds. Check layout in the end of the
  text.

• baseurl in repo file doesn't point to correct location. a) wrong location b)
  $basearch shouldn't be used if there is only one architecture (it would be
  typically noarch and no user has set it as $basearch)

• repofile contains lines which starts with whitespaces

• notification task should receive only .repo files. There is a lost of unused
  arguments now.

• taskRepoNotifications should be builder-only call (host. namespace,
  probably best via export_in decorator)

Builder:

• syntax errors: usage of py2.7 incompatible f-strings over all builder plugin.

• config could be read only once (see save_failed_tree) and should be used
  for both tasks. Use lowercase_names for config items to be consistent
  across all configs.

• don't use requests directly. There is an koji.util.downloadFile function
  which handles some error types.

• mirror_rpms is not a good way to do this. Expect (and put is a default
  channel) that task runs in createrepo channel. In such case you'll get
  access to all those files without downloading them. In such case construct
  pi = PathInfo(topdif=self.options.topdir)

• using mergerepo_c is not safe as only mergerepo could exist at the
  builder. Anyway, it is not that important now, we can revisit it later (or
  document that this plugin needs mergerepo_c installed)

• create_repos can be simplified to if builds: else part. Scratch
  builds will be found by that and non-scratch rpms can be discovered in the
  same way.

• using assert is dangerous as it could be turned off by optimization.
  Better to use logging/exceptions.

• flock part is not needed - no one else touch our work directory. Probably
  an artifact from original code.

• not sure what rm_contents is trying to solve. What is the issue with
  rmtree usage? Another leftover maybe?

• baseurl construction is flawed.

• content_dir is not needed on createrepo builder.

• link_task_repos muse use relative symlinks, so it will work after
  uploading to the hub (or solved some other way)

• E-mail template should refer to koji and should be placed in config.

• recipient check is weird. a) email is always passed through hub creation
  call b) Its length will be always at least 1 "@".

• Don't feed templates with locals() as it will be part of config and could
  leak some information nobody wanted. Be explicit what is propagated there.

• NVR is constructed as N.V.R (in create_repos)- it should be always N-V-R.
  In the other direction (taskRepoNotifications) use koji.parse_NVR or
  koji.parse_NVRA if needed. There could be problems with epoch.

Structure of /mnt/koji/repos-tasks should be:

• official/<name>/<version>/<release>
• scratch/<user>/<name>/<version>/<release>
• tasks/<task_id % 10000>/<task_id> here is a suggested difference to original code from <tasks>/<task_id>

enforce int

It is not checking the version, so it is not needed at all. If command is not present, call will fail anyway with "missing program"-like error.

change one set of ' to "

missing EOLs - results in one long line and createrepo_c will fail on this.

• There is a problem with original plugin - if I run scratch build twice with different sources, but same NVR there will be a conflict when linking to scratch repo which doesn't differentiate. We either have to add task_id into the path or update symlink only to the latest version. In such case, notification must contain also the original task repos, not only symlinked ones, so user can switch accordingly.
• Scratch repo name is missing -scratch suffix (inside .repo file)
• baseurl inside of .repo is not working in noarch case. ($basearch will be e.g. x86_64)

@mikem can you look at this one?

There's a lot here, but let me start with what jumps out.

The hub calls should have access controls. Both are clearly intended to be host-only. Both include a task arg, so they should further be limited to the host running that task.

Also, they are inconsistently exported. Both should be under host.*

The naming is inconsistent. Is it "taskRepo" or "tasksRepo".

The recursive_walk function in the hub code needs work, or possibly to be removed entirely. It shouldn't need to recurse on itself since os.walk already traverses the directory tree. This is the point of using os.walk. Furthermore, I am suspicious of this feature needing a full tree traversal like this. It seems like we should have the information to generate these repo urls more decisively.

The configuration management in the builder plugin seems overly complex. Do we really need two separate config files for this plugin?

Inconsistent config naming: sourcecode vs sourcecodelink

Why are we running mergerepo_c? It seems like we should just generate the repo we want and be done with it.

The policy data seems sparse. Granted, it's not clear what we're trying to accomplish with a policy hook here. Users cannot submit these tasks directly, they are automatically generated by (every) completed buildArch task. I suppose we might want to allow filtering this with policy, but I'm not sure if this is the right place to do that.

A fair chunk of this is the notification portion, which mimics existing notifications in Koji. This isn't the best model to emulate, and I wonder if these notifications are actually required.

Also, it would be a good idea to include unit tests

The builder has a parameter namedtask_id_child, but the task passed in is definitely not a child task. They do not have a parent/child relationship.

@mikem I pinged @veruu about notifications and result is, that we should have enabled notifications as default and add some option to config file for disable it. Are you ok with this?