From the MBS point of view, this looks OK. I will give my +1 after trying it out tomorrow :), but let's give +0.5 now :).

I cannot comment on the way how it is integrated with the rest of rpkg, but it sounds OK to me too.

Sorry about all the rebases. I fixed some grammar in the module overview command and had to fix some Jenkins build errors due to Flake8 errors and EL6 compatibility.

Instead of sys.exit directly from here, it is good to raise rpkgError('mbs-manager is not installed. This is required for local builds.'), fedpkg and other downstream client based on rpkg, e.g. rhpkg, have the ability to catch this error and print like this Could not execute {command name}: 'mbs-manager is not installed. This is required for local builds.'

In package tools built on top of rpkg, the entry point function is responsible for catching error raised from the part of cli and commands, printing error message and then exit.

"""\ is helpful to remove the first \n.

Same as above.

Same as above.

Same as above.

Same as above.

Would be good to give name to value 0 and 3.

I'm using Fedora 25 and mbs-manager cannot be installed from f25 repo. Will it be available, or which Fedora releases and EPEL releases contains mbs-manager?

Would it be too long to sleep 30 seconds? I think that could make user feel command hangs.

I would expect to move module-* commands implementation to pyrpkg.Commands instead of putting them all in cliClient. This is what rpkg implementation to split cli and commands clearly.

An alternative way is patch('sys.stdout', new_callable=StringIO)

Reuse Commands._run_command?

One thing to keep in mind is updating bash completion rules, but that should happen in downstream clients once this is merged.

The reason I didn't here is because I wanted to change the error based on if mbs-manager was installed or not. I will change it.

It's part of the module-build-service RPM. I can add that here instead.

@cqi thank you for the review! I addressed your comments, including moving most of the code to the "Command" class.

:thumbsup: LGTM

One thing to keep in mind is updating bash completion rules, but that should happen in downstream clients once this is merged.

@mprahl as well as the rpkg.mbs config section.

Looks good to me.

Is there any high level documentation how to actually use these commands? As a person that never built modules, what should I do to test this? To be clear I'm not asking you to write this, I'm merely curious if something like that already exists.

Looks good to me.
Is there any high level documentation how to actually use these commands? As a person that never built modules, what should I do to test this? To be clear I'm not asking you to write this, I'm merely curious if something like that already exists.

I'm not aware of any documentation, but it'd be a good idea to have this. Where do you recommend having the documentation? I'll file a ticket in the Factory 2.0 backlog once I find out where to put it.

Is there any high level documentation how to actually use these commands? As a person that never built modules, what should I do to test this? To be clear I'm not asking you to write this, I'm merely curious if something like that already exists.

@langdon's group is coming up with some "internal" docs on how to do this. We should upstream them to a Fedora wiki page asap.

Current status: I think we're just waiting on a :+1: from @jkaluza before merge.

The arguments here are in wrong order, it should be 'autt_method', self.config_section.

Something with watch is wrong here. If I try submitting module build with -w, it submits the build properly, but watch ends up with:

Could not execute module_build: 'Namespace' object has no attribute 'build_id'

Just two comments, otherwise it works for me, so after fixing these, +1

Okay, the two comments were addressed and confirmed by @jkaluza. I squashed the commit but for those who just want to review the change, here is what the commit was:

Author: mprahl <mprahl@redhat.com>
Date:   Tue Oct 17 09:26:29 2017 -0400
    Address feedback
    Signed-off-by: mprahl <mprahl@redhat.com>
diff --git a/pyrpkg/cli.py b/pyrpkg/cli.py
index 604c25a..24c97d1 100644
--- a/pyrpkg/cli.py
+++ b/pyrpkg/cli.py
@@ -1524,7 +1524,7 @@ see API KEY section of copr-cli(1) man page.
                         'required')
         if not self.config.has_option(self.config_section, 'auth_method'):
             raise rpkgError(config_error.format(
-                self.config_section, 'auth_method'))
+                'auth_method', self.config_section))
         required_configs = ['api_url']
         auth_method = self.config.get(self.config_section, 'auth_method')
         if auth_method not in ['oidc', 'kerberos']:
@@ -1565,7 +1565,7 @@ see API KEY section of copr-cli(1) man page.
         """
         self.module_validate_config()
         api_url = self.config.get(self.config_section, 'api_url')
-        self.cmd.module_watch_build(api_url, self.args.build_id)
+        self.cmd.module_watch_build(api_url, build_id)
     def new(self):
         new_diff = self.cmd.new()

Looks good to me. @jkaluza verified it works, so let's merge it.

:thumbsup: merging