#2088 Introduce project-less API token support in pagure-admin
Merged by pingou. Opened by pingou.
admin_tokens  into  master

Download 2088.patch
no initial comment

rebased

I recommend making the above code block a separate function. It'll keep this function smaller and simpler, and it will make each of the functions easier to test.

Same for the above three - if you make each be its own function it'll all be simpler, especially testing.

Why this change?

Why this change?

I recommend documenting what args is.

I recommend documenting what args is.

Same here and in the function below this one.

Why would you want to test argparse? and how? with subprocess?

Because of this https://pagure.io/pagure/pull-request/2088#_1,217

It would be good to document active and expired too.

I recommend writing tests for this pull request.

Indeed :)_

I'm not able to follow that link due to https://pagure.io/pagure/issue/2092

Hm, #2092 is about comment this is a highlight of a line of code, but I would still expect the scrolling to occur, odd :/

The line is highlighted correctly though, so if you click on the link then scroll the diff, it should point you to the line I was referring to

rebased

Pull-request restructured as advised (still short of unit-tests though)

LGTM - the tests are up to you.

Sorry for my original comment, it was out of place.

Thanks for the review, I will not merge right now as I think there are at least some pieces that can be tested simply.

How do you test the bodhi CLI tool yourself? Subprocess?

The bodhi CLI uses click, and click has some nice test integration built in. Here's an example:

https://github.com/fedora-infra/bodhi/blob/2.4.0/bodhi/tests/client/test___init__.py#L43-L49

I know you aren't using click here, but I wanted to show how great it is ☺

Without click, I'd probably design my CLI to be functions that get called rather than just top-level module code. Then the test suite can import the module and do whatever mocking it wants, and call the functions as desired.

rebased

Looking more into testing this code, especially the part about generating the admin api tokens, I see that the code is interactive (asking for user's input) so I'm not quite sure what is the best approach to test this.
Mocking raw_input() doesn't sound very appealing :s

rebased

Hey I went through the code looks interesting but I am not able to understand what exactly this is trying to do can you please explain ? :smile:

The idea is to allow admin to create API token that can be used against any project for certain actions. For example, allow known and trusted 3rd party tools to flag a pull-request.

rebased

rebased

Ok, unit-tests added, I would not mind a review to them :)

LGTM

Thanks! :)

rebased

Pull-Request has been merged by pingou

Metadata