This changes how mbs-manager is called. In what version did its usage change? Should the old version be still supported or is it not usable at all now?

I don't like the removal of skiptests, could we re-add it in MBS side using https://pagure.io/fm-orchestrator/issue/803 and do not remove it here?

I'm also afraid we will need some way in rpkg to detect the old mbs version and use the old API or at least print some error message that the MBS is too old and needs to be updated.

The MBS and rpkg are not bound together anyhow and it is possible to install old MBS and newer RPKG and vice-versa and we should handle that in some nice way.

@lsedlar: What do you think would be the right approach here? We certainly don't want to support the old way in MBS, because it did not work as expected for rpkg users (fedpkg local would for example ignore local changes to modulemd.yaml).

We could somehow detect the old MBS and make the rpkg code more complex to support even older MBS, or we could detect the old MBS and show an error message that mbs needs to be updated in order to do local build.

Or we could change the rpkg RPM to request/suggest the right version of MBS.

@lsedlar @jkaluza
So we could also detect what cli options are set to rpkg. If someone will use the old one they will be converted to the new ones, with a message saying something about deprecation. WDYT?

My vote goes to updating the packaging so that both packages are at the compatible versions and users are forced to update both at the same time. That way no changes in code are needed. New rpkg would require new MBS, and potentially new MBS could conflict with old rpkg (but this may be undesirable if it can be used on its own). Anyway the new packages should go out in the same update.

There's a precedent in fedpkg for keeping compatibility with different versions of other tools: it supports different versions of bodhi command, but there it was an implementation detail invisible to users of fedpkg. This PR changes the usage on rpkg level, so it would be much more difficult. I'm not sure it's worth it.

"branch" => "stream"

Can you verify that file_path is a valid path in an else statement?

Why did you make --file and --stream as options instead of optional arguments?

Why do you have this line?

What if the directory is not named after the module?
For instance, if I did:
git clone https://src.stg.fedoraproject.org/modules/testmodule.git testmodule-stage

Maybe my comment about verifying the modulemd file exists would be better here. Up to you.

Use self.cmd.module_name to get module name.

What is self.repo.working_dir used for? It is not assigned to anything.

Two blank lines here. Looks unnecessary.

@mprahl When will the new mbs-manager, with which this PR should work, be released? Perhaps rpkg with these changes should be released with that mbs-manager at same time in case users get new mbs-manager and old rpkg and then break.

@cqi I'm planning to start the release process today.

My vote goes to updating the packaging so that both packages are at the compatible versions and users are forced to update both at the same time. That way no changes in code are needed. New rpkg would require new MBS, and potentially new MBS could conflict with old rpkg (but this may be undesirable if it can be used on its own). Anyway the new packages should go out in the same update.
There's a precedent in fedpkg for keeping compatibility with different versions of other tools: it supports different versions of bodhi command, but there it was an implementation detail invisible to users of fedpkg. This PR changes the usage on rpkg level, so it would be much more difficult. I'm not sure it's worth it.

I agree. Most people are still using mbs-build (the tool we ported to rpkg), so they can just keep using that while we get both releases out the door.

FYI, here are the Bodhi updates for MBS:
https://bodhi.fedoraproject.org/updates/FEDORA-2017-7d43059e94
https://bodhi.fedoraproject.org/updates/FEDORA-2017-f34d4ad240
https://bodhi.fedoraproject.org/updates/FEDORA-2017-9f86486319
https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2017-54fa0115be

@mprahl if you want to provide a custom named yaml file use the --file option. If you dont provide anything it will search for the module file in the directory with the same name.

@mprahl with the options you see at first glance how we are using the command. Also we don't have to put them in a specific order.

skiptests is a kwarg in the function definition. Can you add that back in the docstring please?

Optional: the rest of the function uses single quotes, it'd be nice to be consistent and use single quotes here and below as well.

Why did skip_tests get renamed to skiptests?

Why was this parameter renamed? This seems like an unneeded breaking change.

Like I said before, I think it'd be nice if the repo name was taken from the remote git URL rather than the cloned directory as I have multiple clones of the testmodule in the same directory, so the cloned directories have different names (e.g. testmodule, testmodule-stage, etc.).

Edit:
You should be able to extract that info from the scmurl.

Can you verify that the file path exists, and if not, return an exception explaining the issue?

Some minor comments, but once addressed, +1.

How about using self.module_name property? That looks at git url and falls back to name in spec file (which will not work here).

+1. I actually commented out for this. We should always get module (the rpkg term) name from module_name property. DRY :) BTW, in cli.py, it's self.cmd.module_name.

@lsedlar @mprahl @cqi

Hi guys,
i'm now using the self.cmd.module_name as the name of the module. As the rpkg always expects to work with an repository, i removed the option to get name for the module file from the working directory. We have the option --file for that. Also i'm checking if the path to the constructed file path exists. + tests.

  --file [FILE_PATH]    The module's modulemd yaml file. If not specified, a
                        yaml file from the current working dir will be used,
                        with the same basename as the current working
                        direrctory.

According to --file description, I thought, with given --file, module-build-local might be able to run outside a module repo, but it is not true. I'm not sure if this is the expected behavior. Otherwise, looks good to me.

"direrctory" => "directory"

This help is now inaccurate since you now use self.cmd.module_name.

I would use self.cmd.path here instead because rpkg supports overriding the path to run the command in with --path.

Optional: It'd be nice to continue using single quotes (e.g. ') in the test instead of double quotes (e.g. "). It's a bit confusing going between the two.

Optional: It'd be nice to continue using single quotes (e.g. ') in the test instead of double quotes (e.g. "). It's a bit confusing going between the two.

Optional: It'd be nice to continue using single quotes (e.g. ') in the test instead of double quotes (e.g. "). It's a bit confusing going between the two.

Optional: It'd be nice to continue using single quotes (e.g. ') in the test instead of double quotes (e.g. "). It's a bit confusing going between the two.

Optional: It'd be nice to continue using single quotes (e.g. ') in the test instead of double quotes (e.g. "). It's a bit confusing going between the two.

Optional: It'd be nice to continue using single quotes (e.g. ') in the test instead of double quotes (e.g. "). It's a bit confusing going between the two.

"and exception" => "an exception"

"dont" => "don't"

If you follow my comment from diff line 37 in pyrpkg/cli.py, this mock will not be needed anymore.

If you follow my comment from diff line 37 in pyrpkg/cli.py, this mock will not be needed anymore.

Same spelling mistakes as above

If you follow my comment from diff line 37 in pyrpkg/cli.py, this mock will not be needed anymore.

I think it'd be cleaner to just mock os.path.isfile.

I think it'd be cleaner to just mock os.path.isfile.

I think it'd be cleaner to just mock os.path.isfile.

@mcurlej, I'm done reviewing. Please address the comments and let me know when you're ready for another round of review. I'm sorry for missing some of these in a previous review, but I've since learned more of the code base in rpkg/fedpkg since the last review.

@mprahl fixed the typos, changed the quotes to single qoutes, using self.cmd.path. Could not mock self.path.isfile as it will cause problems in the codebase and fire exception elsewhere (during the vailidation the git lib will fire an exception) so i left it as is.
@cqi Fixed the help desc.

+1

Looks good to me.

This one looks to have sufficient :+1:s. Can it be merged?

Yup, merging :tada: