These three urls could move to three separated dedicated section fedpkg.bugzilla, fedpkg.pagure, fedpkg.pdc. I think this separation gives a clear view of the core options and other services' options that are used to work with.

User configuration file is really useful. I like it. Instead of handling a user configuration file separately, why not let fedpkg read it along with system-wide config file /etc/rpkg/fedpkg.conf together? configparser can handle these files properly.

Based on above comment, in /etc/rpkg/fedpkg.conf, there is

[fedpkg.pagure]
url = 'https://pagure.io/

in ~/.config/rpkg/fedpkg.conf, there is

[fedpkg.pagure]
token = <api_key_here>

after reading, we can access both of these pagure options.

(to be continued) we don't need to pass user_config explicitly as user configuration is already included in config.

And also don't need to check existence of user configuration for these two commands here. Just call get_pagure_token inside each command, raising error if someone forgets to create his own config file with token.

This is what I'm thinking that we can reuse existing fedpkg code without handling user configuration and options check for request_repo, request_branch separately. What do you think?

pretty please pagure-ci rebuild

Is it worth to move this to /etc/rpkg/fedpkg.conf fedpkg section?

Another user case would be to validate user input namespace. For example, rhpkg is able to clone in form rhpkg clone namespace/repo-name, so the namespace part could be validated and report error earlier rather than relying on git to report a clone error. This is what I can imagine now. Any other can benefit from this?

/cc lsedlar

This is just an idea and no need to do in this PR.

What do you think guys? :)

Good idea

It can't hurt. I'll move it to the config file.

@cqi thank you for the review. I've addressed your comments in the latest rebase.

Looks good to me. Thanks.

@mprahl is it ok to try these two commands with fedpkg-stage?

Looks good to me. Thanks.
@mprahl is it ok to try these two commands with fedpkg-stage?

Should be totally fine to test it out. By the way, I'll be away starting December 14th and coming back January 2nd. Is it possible to get this merged before then?

I also recorded a demo showing the basic functionality of this PR:
https://fedorapeople.org/groups/factory2/demos/mprahl-fedpkg-request-repo-branch.webm

--user--config does not appear in -h output, e.g. fedpkg -h.

s/--user--config/--user-config/

Generally the patch looks good to me. The tests are very nice. I'm still not completely convinced the package name should always be required for requesting a new branch, but I guess it's not a big deal.

I think @lsedlar is correct. I didn't notice this. Firstly, --module-name can be reused to specify package name, that option is actually used for the purpose of overriding the behavior of getting package name from git push URL. So, following forms could work

fedpkg --module-name new-package request-repo
fedpkg --module-name new-package request-branch ...

Secondly, for requesting a branch, there would be two scenario. One is to request branches without waiting for the repo to be approved and created. Another one is someone could request new branches inside a clone repository. In the latter case which I mentioned in #137 , package name, the repo argument, is not necessary actually. Just use Commands.module_name. An exmaple is

fedpkg request-branch branch

@mprahl Could you reconsider this?

@cqi I prefer keeping it the way it is, but since you both prefer it, I'll incorporate your recommendation.

Okay, I addressed the comments. Please review again.

Fixed

@mprahl Thank you very much. Looks good. :thumbsup:

I also find some issues

• --all-releases does not create request for EPEL branches el6 and epel7

• --all-releases creates duplicate request if that was created already, steps to reproduce

fedpkg-stage --module-name esteidcerts request-branch f27
fedpkg-stage --module-name esteidcerts request-branch --all-releases

then there is duplicate request for branch f27

• request-branch creates duplicate request as well, steps to reproduce

fedpkg-stage --module-name esteidcerts request-branch f27
run again

there are two requests for branch f27

I think these issues do not impact the use of this first version. Let's fix them later.

@mprahl Thank you very much. Looks good. 👍
I also find some issues

--all-releases does not create request for EPEL branches el6 and epel7

--all-releases creates duplicate request if that was created already, steps to reproduce

fedpkg-stage --module-name esteidcerts request-branch f27
fedpkg-stage --module-name esteidcerts request-branch --all-releases

then there is duplicate request for branch f27

request-branch creates duplicate request as well, steps to reproduce

fedpkg-stage --module-name esteidcerts request-branch f27
run again

there are two requests for branch f27
I think these issues do not impact the use of this first version. Let's fix them later.

@cqi, it was by design that --all-releases doesn't include EPEL because most packages don't go to EPEL, and the ones that do, don't necessarily need both EPEL6 and EPEL7.

As to your other comment, I'm okay with it not being idempotent. To make it idempotent, we'd either keep track of what the user has requested, or check all open requests by the user in the Pagure ticket queue. The former would be error prone since the user may want to resubmit that same request if the first request was denied by an admin by mistake, and the latter adds additional API calls to Pagure, which would slow the down the performance of the command even more. We could look into implementing the latter option, but I'd like to hold off.

With that said, thank you very much for the review. May you please merge it when you feel comfortable?

@cqi is this good to merge?

Code wise this looks good to me.

Thank you all :tada: