I know that the proposal doesn't say everything and the implementation could be bad even though the rough design looks good or otherwise, but I want to ask you whether there is anything that you strongly disagree with? Are there any missing features of APIv1 or APIv2 that you like and weren't considered for APIv3 design?

After we agree on the design proposal, I would like to implement a small piece of APIv3 and we can talk about the code. Then, rest of the API should be implemented piece by piece, entity by an entity so we can re-assure that we have the implementation that we want and not repeat the past.

I thought we would just extend API1 to support json, clean the code a little bit (and maybe even the interface) and we are done. And API2 would be deprecated. That way we could at least (more less) keep compatibility with Koschei client that exists out there. If there should be APIv3, it would be cool to see Pyhon code snippets (similar to http://python-copr.readthedocs.io/en/latest/client_v1/Examples.html#work-with-builds) that would show how one will work with the the new python API (so that we can make sure it's really better than APIv1) :).

I had to study the api_v2 a bit to appreciate the convenience of the API, and I'm not able to decide what my final opinion is :-) (v1 vs. v2 vs. v3), my thoughts:

• schemas on cli side are OK, schemas on FE duplicate models/forms as @frostyx pointed out
• I'm lost why on client side are resources + handlers + entities needed, it seems to me like handlers + entities would be enough.. (or if renamed, resources + entities). So I agree here, too.
• it would really nice to have abstract.py having all the abstract classes, and have per-route file on client side, say project.py which would bring all the related classes spread among several files, +1 on that, too
• overall, from user perspective - the api_v2 seems to be a bit nicer to me, and modern, consistent, objective, db-like feeling
• from maintainer perspective, api_v1 is easier to hack on (but I wouldn't say it is easier to keep it compatible)

Thank you guys for the feedback.

I thought we would just extend API1 to support json, clean the code a little bit (and maybe even the interface) and we are done. And API2 would be deprecated.

This would probably be the fastest solution, but I am afraid that we can't do that if there are going to be changes of the data format, such as moving from current POST fields to JSON. We would have to extend the API (as you said) to support both, deprecate the current format and later touch on the API again to remove its support completely. I would prefer to create APIv3, deprecate both APIv1 and APIv2 and let them work for another few months (even half a year). However without adding support for them or fixing bugs in them. Recommendations for these situations should be "APIvX is deprecated, please move on to APIv3".

That way we could at least (more less) keep compatibility with Koschei client that exists out there.
I've never heard of this, so I didn't consider it in the proposal at all. Can you please point me to the client, if you know the URL?

If there should be APIv3, it would be cool to see Pyhon code snippets [...] that would show how one will work with the the new python API (so that we can make sure it's really better than APIv1) :).

Sure, 100% agree here. I am currently working on them :-)

I had to study the api_v2 a bit to appreciate the convenience of the API, and I'm not able to decide what my final opinion is :-)

You are not alone, this is kind of a theme of the last year. Thank you for sharing the thoughts ^^

I imagine the following code samples (I've modified some of the linked ones)

# building new package
builds = cl.builds.create_from_urls("copr1",
    urls=["http://example.com/pkg.src.rpm"])

# cancel the created build - option 1
for build in builds:
    cl.builds.cancel(build)

# cancel the created build - option 2
# a sexy way how to do it, but in this moment I am not sure if we can do it without having
# such complex code as APIv2 has. So far just count on the first option
for build in builds:
    build.cancel()

# get build status for each chroot
for build in builds:
    print("build: {0}".format(build.id))
    for chroot in client.builds.get_chroots(build):
        print("\t chroot {0}:\t {1}".format(chroot.name, chroot.status))

Basically, the main thing that will happen is namespacing per resource (or whatever you want to call it). So instead of client.create_new_build_url(...) we will switch to client.builds.create_from_url(...), or for example from client.create_project(...) to client.projects.create(...).

We may also change the original result.builds_list and return the list of created builds right away.

We may also change the original result.builds_list and return the list of created builds right away.

Actually, I am thinking about this for a long time and I am still not sure which way is better. Have let's say client.builds.create_from_urls(...). What should it return?

Option 1 is a list of created builds. It's user-friendly, you get what you want right away. If build creation wasn't successful, we can raise an exception.

Option 2 is some Response object. You would have to to do something like response.data to get the builds, but we would have a nice possibility to add information to the response. Like some message from frontend, information about the API endpoint (whether is it for example deprecated or tech preview), ...

What are your thoughts about this?

I imagine the following code samples (I've modified some of the linked ones)

# building new package
 builds = cl.builds.create_from_urls("copr1",
     urls=["http://example.com/pkg.src.rpm"])

I think I would prefer cl.Build.create_from_urls. Like we have a class Build with a class method create_from_urls.

# cancel the created build - option 1
for build in builds:
cl.builds.cancel(build)

# cancel the created build - option 2
# a sexy way how to do it, but in this moment I am not sure if we can do it without having
# such complex code as APIv2 has. So far just count on the first option 
 for build in builds:
     build.cancel()

So, this would be really nice. Is there anything preventing us from doing this? The class Build would have class methods create_from_urls as well as instance methods (cancel).

# get build status for each chroot
for build in builds:
     print("build: {0}".format(build.id))
 for chroot in client.builds.get_chroots(build):
      print("\t chroot {0}:\t {1}".format(chroot.name, chroot.status))

Again, build.get_chroots() would be nicer I think (if it is possible).

Basically, the main thing that will happen is namespacing per resource (or whatever you want to call it). So instead of client.create_new_build_url(...) we will switch to client.builds.create_from_url(...), or for example from client.create_project(...) to client.projects.create(...).
We may also change the original result.builds_list and return the list of created builds right away.

I don't know what it returns now but I would aim for the most natural behavior possible.

cl.Build.create_from_urls I guess does not make so much sense. It would rather be Build.create_from_urls(cl, "copr1", urls=["http://example.com/pkg.src.rpm"]) so you would pass the "client" (I think you would rather pass something like config or connection_data into that class method for constructing the build objects. Build object then will have access to those data so they invoke remote calls as they wish.

I am just brainstorming here. The main problem is how the connection data (api url, tokens) will be handled.

Thank you @clime,

I think I would prefer cl.Build.create_from_urls. Like we have a class Build with a class method create_from_urls.
So, this would be really nice. Is there anything preventing us from doing this? The class Build would have class methods create_from_urls as well as instance methods (cancel).

Cool, I've also been thinking about this. Seems to be the easiest way how to achieve it. However, I don't really like the capital B here cl.Build.create_from_urls. I know, it is a class, it should be capital, but it is really user unfriendly because everyone will just type cl.b and then press tab and wonder why it is nothing there. I would prefer to do something like this (just pseudocode)

from resources.builds import Builds
class Client:
    def __init__(self, ...)
        self.builds = Builds

This way one could do client.builds.create_from_urls(...) ... What do you think? Also, personally I don't really care whether it is Build or Builds. But you are probably right, that it should be just Build because then it would make more sense if you have an object of this class and do build.cancel().

I don't know what it returns now but I would aim for the most natural behavior possible.

Which is? I think that natural behaviour would be something different for everyone :-). Currently the APIv1 returns responses and APIv2 returns objects right away.

This way one could do client.builds.create_from_urls(...) ... What do you think? Also, personally I don't really care whether it is Build or Builds. But you are probably right, that it should be just Build because then it would make more sense if you have an object of this class and do build.cancel().

Ye but that's just a hack. We shouldn't be doing things like that. Like you said, its a class - it should have a capital letter at the beginning. The main issue is we are trying to make that class accessible under client instance. Really Build.create_from_urls(cl, "copr1", urls=["http://example.com/pkg.src.rpm"]) comes to mind then but it's slightly inconvenient to pass the client (rather connection config) into each object-instance create method.

Which is? I think that natural behaviour would be something different for everyone :-). Currently the APIv1 returns responses and APIv2 returns objects right away.

Probably objects. Or really just Munches without any kind of abstraction (and attached method). I wouldn't object if the new API was really just a tiny wrapper over remote calls without any "resources" or "handlers" (and other not very useful notions). That seems to be an interesting alternative.

One issue that I see with that other "high-level" approach (Build class, Build instances) is that e.g. build.status property does not give you the current build status (unless you invest into programming it that way). It just gives you the cached data from the moment when the object was created at the client side.

So from this point of view, it seem to me that having some client methods (as wrappers over remote calls) that return some dictionaries (or munches) kind of avoids this problem (it's a problem of expectations. The programmer is used to the instance attributes to have up to date values :) but here we are really working only with some "cached" objects that represent objects on the remote side).

But I didn't invest time to really study the current APIs and do some investigation over its use so take all this just as a very quick feedback and braindump.

I've implemented more of the client code samples based on this discussion
https://gist.github.com/FrostyX/46206d61b69033fdd04b4e953ec40b89

There are sketches of both high-level and tiny wrapper approaches.

I've implemented more of the client code samples based on this discussion
https://gist.github.com/FrostyX/46206d61b69033fdd04b4e953ec40b89
There are sketches of both high-level and tiny wrapper approaches.

This is very cool what you did there.

But note that all of those approaches have namespacing in them (client.builds, client.Build etc.). I consider that namespacing itself not being "pythonic" (we can discuss it with python Gurus if you want, this is just my personal feeling).

Anyway, can you do additional approaches without the namespacing? Note that what I previously suggested Build.create_from_urls(cl, "copr1", urls=["http://example.com/pkg.src.rpm"]) is also not good for the high-level approach. We would need additional class except Build that would accept the client (or config) instance and that would provide the "static" methods (they will no longer be static) like create_from_urls.

My note on Build vs. build: client.build is a method (therefore usually lowercase) which returns class (which stays uppercase). I have never seen an example of a function which would be in uppercase just because it returns object. And there are plenty of such methods.

Anyway, can you do additional approaches without the namespacing?

Sure. I must confess that so far I am not a big fan of this option, but hey, that is exactly why I created the gist ^^. I will create some code samples and hopefully, it will show us better :-)

But note that all of those approaches have namespacing in them (client.builds, client.Build etc.). I consider that namespacing itself not being "pythonic" (we can discuss it with python Gurus if you want, this is just my personal feeling).

Imho namespacing is very pythonic, but commonly it is rather done on modules/files level when it is enough (instead of for example static classes).

My comments as koschei developer:

• Koschei copr plugin is not used in production yet, so I'm ok with changing it
• I started koschei-copr-plugin with APIv2, then moved to APIv1, so I can comment on both.
• What I don't like about APIv2:
  • Reliance on numeric IDs. I'd like to acces objects by their natural key. I want to get projects by owner/name. It makes everything simpler. Look at other APIs out there. Does GitHub require you to fetch some arbitrary internal numeric ID to be able to get project releases? No. Does pagure? No.
  • The object-oriented interface is not well suited for our use-case. Function-based interface of APIv1 worked much better for us. Here's why:
    • It requires you to make unnecessary get requests. When you want to edit a entity, you have to get it first, even if you don't need it for anything (you have the primary key in the DB). When doing multiple operations, this doubles the amount of requests and slows the application down.
    • It requires you to fetch parent entities to be able to manipulate children. So you have to get project to be able to edit a chroot (even if you know the primary key already, as in the previous point). That adds another unnecessary request.
• I understand that for simple scripts the object-oriented API is more convinient to use, but not for services that cannot afford to waste time on fetching the same project over and over just to satisfy requirements of the wrapper. IMO it shouldn't be hard to have both function-based and object-oriented interfaces. chroot.update() instance method could delegate to Chroots.update(**primary_key, **updated_attributes) function internally.
• What I dislike about APIv1:
  • Inconsistent and verbose argument naming. I like to do calls with keyword arguments, but once it's username, another time ownername. Should be just owner, IMO.
• What I like about APIv1:
  • It is simple and intuitive
• What I dislike about both APIs:
  • They both provide different subsets of the functionality available via web. Some things are not available in APIv1, some things are not available in APIv2. Good API should be able to do everything that you can do with web frontend.

Thank you @msimacek for the feedback. Koschei plugin was mentioned in the discussion, so I am very happy to see its developer here :-).

Reliance on numeric IDs. I'd like to acces objects by their natural key. I want to get projects by owner/name. It makes everything simpler. Look at other APIs out there. Does GitHub require you to fetch some arbitrary internal numeric ID to be able to get project releases? No. Does pagure? No.

Sounds like it is much easier to get_list of all projects with particular owner/project values, which is exactly one project and then get the object on [0] index, than querying just one project. I get the ponit of not relying on numeric IDs.

It requires you to make unnecessary get requests.
It requires you to fetch parent entities to be able to manipulate children.

Number and time of the requests probably nobody considered, thank you for pointing it out.

Inconsistent and verbose argument naming. I like to do calls with keyword arguments, but once it's username, another time ownername

+1 it is really annoying. In the past, there weren't group projects in Copr, so username was a good name, but then group projects became a thing and code became a bit messy.

They both provide different subsets of the functionality

Yep, that is true. We wanted to deal with it and implement the missing functionality, but we weren't sure into which API it should be implemented. Now we finally made a move and therefore this discussion.

I would like to write some thoughts on "namespacing or not" topic.

I've taken the list of APIv1 client methods (rather than API endpoints as it was originally suggested because the discussion about namespacing was regarding the client object and user-readability) and compared it with how it would look like namespaced (not a final version ofc).

authentication_check(...)
get_build_details(...)
cancel_build(...)
delete_build(...)
create_new_build(...)
create_new_build_pypi(...)
create_new_build_tito(...)
create_new_build_mock(..)
create_new_build_scm(...)
create_new_build_rubygems(...)
create_new_build_distgit(...)
edit_package_tito(...)
add_package_tito(...)
edit_package_pypi(...)
add_package_pypi(...)
edit_package_mockscm(...)
add_package_mockscm(...)
edit_package_scm(...)
add_package_scm(...)
edit_package_rubygems(...)
add_package_rubygems(...)
get_packages_list(...)
get_package(...)
delete_package(...)
reset_package(...)
build_package(...)
get_project_details(...)
delete_project(...)
fork_project(...)
create_project(..)
modify_project(...)
get_projects_list(...)
edit_chroot(...)
get_chroot(...)
get_project_chroot_details(...)
search_projects(...)
get_build_config(...)
get_module_repo(...)
build_module(...)

VS namespaced

api
    authentication_check(...)

project
  get(...)
  create(..)
  delete(...)
  modify(...)
  fork(...)
  search(...)
  get_list(...)
  get_chroot(...)
  get_chroot_details(...)
  edit_chroot(...)
  get_build_config(...)

package
  get(...)
  delete(...)
  reset(...)
  build(...)
  create(...)  # Should replace add_package_*
  edit()  # Should replace edit_package_*
  get_list(...)

build
  get(...)
  cancel(...)
  delete(...)
  create(...)
  create_from_pypi(...)
  create_from_tito(...)
  create_from_mock(..)
  create_from_scm(...)
  create_from_rubygems(...)
  create_from_distgit(...)

module
  build(...)
  get_repo(...)

Personally, I am all for the namespaced version. It doesn't necessarily have to be available via client object, e.g. client.build.get(...), I might also agree with build_r = BuildResource(config); build_r.get(...). I just very much dislike the not-namespaced list of methods packed in one file like we have in APIv1.

@clime, as you pointed out, that in your opinion client.build is confusing because you think that the build looks to you like a particular build instance, not a resource object, then we should IMHO try to figure out other (better) options how to do it.

I mean, when looking at the namespaced list, the methods look much cleaner and shorter.

• delete_project vs delete
• create_new_build_scm vs create_from_scm
• get_projects_list vs get_list

Anyway, can you do additional approaches without the namespacing?

I've added some, can you look at them, please? Ofc there is an obvious solution to doing this and it is a Client class with a bunch of methods just like we have in APIv1. It is a valid option, but I just didn't add the code sample because we all know how it looks like. Instead, I've added a code sample of how to have the same Client interface, but methods separated across multiple files (maybe there is a bit cleaner way how to do it, we can discuss it if this option is the most attractive for someone. And also there is a sample of passing config into resource class without having a Client object at all - which doesn't actually look that bad as I originally expected. But in this case, having a client object isn't a mutually exclusive idea, ...

Ok, i admit that the namespaced list looks much better. Note that I wouldn't put stuff like

  create_from_tito(...)
  create_from_mock(..)
  create_from_distgit(...)

into the new api. It's already deprecated now.

But I am e.g. not sure how you want to implement.

package.edit

from the second table. I think you need something like package.edit_rubygems unless the package edit function accepted source_type and source_json_dict, which I guess is a possibility.

But note that you could then also simplify the first list if that change was made (meaning that the comparison you made is not completely fair).

client.build is confusfing, yes. That's because I would e.g. expect I can write things like client.build.delete() (meaning that the functions like delete or cancel do not have any parameters there but they do have them because build is not actually a build instance but just a build function namespace).

In other words you need to have some context knowledge there. It's not like you come to that code and start typing things that you are normally used to.

BuildResource(config) etc. has an disadvantage that you additionally need e.g ProjectResource(config) when you want to work with both builds and projects. Also the *Resource naming is probably not the best ever.

There also might be a better (more explicit) name for build in that client.build expression. Maybe we could call it something else while still keeping the advantage that the client can pre-create the instances for each namespace?

As for the other comment...frankly, I don't really mind having one file with quite a lot of methods in this case. I think we could make those methods pretty short (just really a few lines) and then we could add visual separators there like (##### build methods #####). On the other hand, I quite like the separation you have made in that second list (but my humble objection about client.build and client.builds remains). The main problem of api1 is its inconsistency in argument (and perhaps also function) naming. That namespacing thing is something extra but I admit it might be nice to have.

BuildResource(config) etc. has an disadvantage that you additionally need e.g ProjectResource(config) when you want to work with both builds and projects.

This might be not be such a big deal. Mainly the naming would deserve some more thought in my opinion.

Thank you for the feedback @clime. I basically agree with most of the points that you made in the previous comments, so I am not going to quote every one of them and +1 it.

client.build is confusfing, yes. That's because I would e.g. expect I can write things like client.build.delete() ...
In other words you need to have some context knowledge there.

Well, I agree just partially. It allows writing an ugly code like

build = client.build
...
build.cancel(...)

Then I would expect the build to be some Build object with cancel method on itself. Another example of bad code would be

foo = Client()
foo.build.cancel(...)

Then I have no idea what foo is. It might be some weird construct providing some Build object with cancel method on itself (without passing arguments into it). So yes, you need context.

Those are examples of a bad code. This code

copr_client = Client()
copr_client.build.cancel(...)

is IMHO clear and looks like ... I can't remember the right design pattern, maybe proxy ... object provided by copr_client. Therefore I would expect some build-specifying params there.

But I guess that can be a matter of opinion.

Also the *Resource naming is probably not the best ever.
There also might be a better (more explicit) name for build in that client.build expression. Maybe we could call it something else while still keeping the advantage that the client can pre-create the instances for each namespace?

This is what I want to talk about. I agree with you, there might be a better naming. Anything that comes to your mind? Looks like solving this would make us closer to a solution that all of us will agree with.

and then we could add visual separators there like (##### build methods #####).

If something does so many things that it starts to be hard to orient in, or even there is a need to group some actions together and add a visual separator, then I think that it really asks to be solved on code-level. I mean sure, visual separation is much better than nothing, but it can be IMHO done better.

But I can see that you like the idea of namespacing too, you just disagree with the naming. Which is totally fine, we should focus on that.

is IMHO clear and looks like ... I can't remember the right design pattern, maybe proxy ... object provided by copr_client. Therefore I would expect some build-specifying params there.

Ok, so what about calling those objects build_proxy, project_proxy, ...? That could actually be a really good name. Note that it would be nice to able to instantiate the respective classes of those objects even without using the Client class. I would like to be able to write e.g. build_proxy = BuildProxy(config) just like that. Client might be there or might not be there.

If something does so many things that it starts to be hard to orient in, or even there is a need to group some actions together and add a visual separator, then I think that it really asks to be solved on code-level. I mean sure, visual separation is much better than nothing, but it can be IMHO done better.

The problem is that the "code" separation is mostly only good for us. There are not so many methods for one class to render code auto-completion unusable. And we can do the separation (Client build methods, Client project methods, etc.) in docs so that a user can navigate more easily.

The advantage of this is that the python api would be 1:1 to the actual url entry points that frontend offers (/api/ views have no grouping currently even though they could have...), the method names in our frontend api would be the same as in the python api, and also it would make us want to make the methods as short as possible :).

I actually think both approaches are okay if implemented properly. So it's really your choice.

Ok, so what about calling those objects build_proxy, project_proxy, ...?

I am really not sure whether it is by definition a proxy. Is there a commonly accepted definition anyway? Although, I think, that if any design pattern matches this situation, it should be a proxy. @msuchy, can you confirm? If yes, I agree with this naming convention.

Note that it would be nice to able to instantiate the respective classes of those objects even without using the Client class. I would like to be able to write e.g. build_proxy = BuildProxy(config) just like that. Client might be there or might not be there.

Exactly, I really like this idea

The problem is that the "code" separation is mostly only good for us

This is important though. APIv2 also feels a bit better to the user, but no one really wants to touch the code. I wouldn't necessarily force something that is really unfriendly for the user, but nice for us. I really think that in this case, it is just a matter of an opinion. I personally as a user would prefer the client.project.create(...) over client.create_project(...).

I actually think both approaches are okay if implemented properly. So it's really your choice.

Thanks, I would like to start with some namespacing. If the code-review shows that it sux, we can fall back to non-namespacing option, but I believe that it won't be necessary.

LOCAL DEPLOYMENT:

Merge into current master went without problems.

$ cd copr/python
$ rpkg local
copr/test/client_v3/test_requests.py ....F                               [100%]
>       assert ex.value.message == "Something wasn't found"
E  AttributeError: 'CoprRequestException' object has no attribute 'message'

I commented out the assert in test_requests.py. rpkg local ran successfully after this and I could install the python3-copr package.

PLAYING AROUND:

>>> from copr.client_v3 import BuildProxy, PackageProxy
>>> from copr import BuildProxy
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: cannot import name 'BuildProxy'

why are Proxies accessible through the client module only? client module, in case we really want it, should only supply the Client class, an instance of which aggregates proxies for easier access to them. It's the same e.g. here:

>>> from copr.client_v3.pagination import Pagination
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'copr.pagination'

Why not simply:

>>> from copr.pagination import Pagination

?
...

>>> m = Munch({'username': 'clime', 'login': 'ppoonybxsbhqldybrkrd', 'token': 'wbnsbwdvtbrtfzwsfvplqusqbynkaa', 'copr_url': 'http://localhost:8080'})
>>> build_proxy = BuildProxy(m)
>>> build_proxy.get(732232)
Munch({'built_pkgs': ['rpkg 2.2'], 'chroots': {'fedora-27-x86_64': 'succeeded'}, 'ended_on': 1525692330, 'id': 732232, 'owner': 'clime', 'project': 'jjjkkk-fork', 'results': 'http://localhost:5002/results/clime/jjjkkk-fork', 'results_by_chroot': {'fedora-27-x86_64': 'http://localhost:5002/results/clime/jjjkkk-fork/fedora-27-x86_64/00732232-rpkg-util/'}, 'src_pkg': '', 'src_version': '2.2-1.fc28', 'started_on': 1525692129, 'status': 'succeeded', 'submitted_by': 'clime', 'submitted_on': 1525692124, '__response__': <copr.client_v3.requests.Response object at 0x7f71d03abe80>})

Looks nice!

>>> dir(build_proxy)
['__class__', '__delattr__', '__dict__', '__dir__', '__doc__', '__eq__', '__format__', '__ge__', '__getattribute__', '__gt__', '__hash__', '__init__', '__init_subclass__', '__le__', '__lt__', '__module__', '__ne__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__sizeof__', '__str__', '__subclasshook__', '__weakref__', '_create', 'api_base_url', 'auth', 'cancel', 'config', 'create_from_custom', 'create_from_file', 'create_from_pypi', 'create_from_rubygems', 'create_from_scm', 'create_from_url', 'create_from_urls', 'delete', 'get']

I am missing get_list there. Grepping the code I can see:

package.py:    def get_list(self, ownername, projectname, pagination=None):
project.py:    def get_list(self, ownername, pagination=None):

Why the build_proxy does not have get_list as well?

>>> build_proxy.delete(732232)
Munch({'built_pkgs': ['rpkg 2.2'], 'chroots': {'fedora-27-x86_64': 'succeeded'}, 'ended_on': 1525692330, 'id': 732232, 'owner': 'clime', 'project': 'jjjkkk-fork', 'results': 'http://localhost:5002/results/clime/jjjkkk-fork', 'results_by_chroot': {'fedora-27-x86_64': 'http://localhost:5002/results/clime/jjjkkk-fork/fedora-27-x86_64/00732232-rpkg-util/'}, 'src_pkg': '', 'src_version': '2.2-1.fc28', 'started_on': 1525692129, 'status': 'succeeded', 'submitted_by': 'clime', 'submitted_on': 1525692124, '__response__': <copr.client_v3.requests.Response object at 0x7f71d03ab7b8>})
>>> build_proxy.delete(732232)
Traceback (most recent call last):
...
copr.client_v3.exceptions.CoprRequestException: Build 732232 does not exist.

Looks good.

>>> build_proxy.create_from_scm(clone_url='https://pagure.io/rpkg-util', ownername='@copr', projectname='copr')
Munch({'built_pkgs': None, 'chroots': {'epel-7-ppc64le': 'waiting', 'epel-7-x86_64': 'waiting', 'fedora-26-ppc64le': 'waiting', 'fedora-26-x86_64': 'waiting', 'fedora-27-ppc6

Again, nice interaction (I will be saying OK from now on).

>>> build_proxy.cancel(732233)
Munch({'built_pkgs': None, 'chroots': {'epel-7-ppc64le': 'canceled', 'epel-7-x86_64': 'canceled', 'fedora-26-ppc64le': 'canceled', 'fedora-26-x86_64': 'canceled', 'fedora-27-

OK.

>>> build_proxy.delete(732233)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/clime/copr/python/copr/client_v3/proxies/build.py", line 202, in delete
    response = request.send()
  File "/home/clime/copr/python/copr/client_v3/requests.py", line 50, in send
    handle_errors(response.json())
  File "/usr/lib/python3.6/site-packages/requests/models.py", line 892, in json
    return complexjson.loads(self.text, **kwargs)
  File "/usr/lib64/python3.6/site-packages/simplejson/__init__.py", line 516, in loads
    return _default_decoder.decode(s)
  File "/usr/lib64/python3.6/site-packages/simplejson/decoder.py", line 374, in decode
    obj, end = self.raw_decode(s)
  File "/usr/lib64/python3.6/site-packages/simplejson/decoder.py", line 404, in raw_decode
    return self.scan_once(s, idx=_w(s, idx).end())
simplejson.scanner.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

NOT OK.

>>> build_proxy.create_from_pypi('motionpaint')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: create_from_pypi() missing 2 required positional arguments: 'projectname' and 'pypi_package_name'

I wonder...what about having ownername and projectname part of **kwargs and merge them with config. Meaning, that you could put ownername and projectname into the config param and then not need to specify them when creating the build? What do you think about it?

>>> build_proxy.create_from_pypi('@copr', 'copr', 'motionpaint')
Traceback (most recent call last):
...
copr.client_v3.exceptions.CoprRequestException: Bad request parameters: {'python_versions': ["'3 2' is not a valid choice for this field"]}

NOT OK.

I wasn't able to start PyPI build for both python versions at once.

Finally, this worked:

>>> build_proxy.create_from_pypi('@copr', 'copr', 'motionpaint', python_versions='2')

>>> build_proxy.create_from_rubygems('A_123')

Again, it would be nice if this just worked.

>>> build_proxy.create_from_custom('@copr', 'copr', script='echo foo')
Traceback (most recent call last):
...
copr.client_v3.exceptions.CoprRequestException: Bad request parameters: {'chroot': ['Not a valid choice']}
>>> build_proxy.create_from_custom('@copr', 'copr', script='echo foo', chroot='fedora-28-x86_64')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: create_from_custom() got an unexpected keyword argument 'chroot

There seems to be some problem with the chroot param.

Now as for package_proxy:

>>> package_proxy.get_list()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: get_list() missing 2 required positional arguments: 'ownername' and 'projectname'

Again, it would be nice to able to specify ownername, projectname as part of config.

>>> package_proxy.get_list('@copr', 'copr')
[Munch({'copr': 'copr', 'enable_net': False, 'id': 30744, 'name': 'copr-backend', 'old_status': None, 'owner': '@copr', 'source': {}, 'source_type': 'unset', 'webhook_rebuild...

Nice.

>>> len(package_proxy.get_list('@copr', 'copr'))
32

That's the correct count.

>>> package_proxy.get('@copr', 'copr', 'copr-frontend')
Munch({'copr': 'copr', 'enable_net': False, 'id': 30747, 'name': 'copr-frontend', 'old_status': None, 'owner': '@copr', 'source': {}, 'source_type': 'unset', 'webhook_rebuild': False, '__response__': <copr.client_v3.requests.Response object at 0x7f71cdfcc048>})

We should be thinking about removing 'old_status' there. Not necessarily in this PR but we should polish the returned results so that they always make sense before the new API is published.

>>> package_proxy.delete(30747)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: delete() missing 2 required positional arguments: 'projectname' and 'packagename'

build_proxy.delete method accepts id as an argument. We should try to unify this if possible. Consider calling this method delete_by_name, accepting a single param name (if ownername, projectname are predefined):

package_proxy.delete_by_name('copr-frontend')

Proxy should be actually an abstract class (interface) defining the interface for the individual concrete proxy classes (BuildProxy, PackageProxy,...). In that class, there should be something like:

def delete(id):
"""
@param id: id of the object to be deleted
"""
raise NotImplemented()

So if we want to delete by ids and by names as well, we likely need to separate the methods.

It would be good to have the class Proxy with all the base methods explicitly defined if it is not yet.

Looking further at the PackageProxy methods like get or edit, they also use package_name as a param so delete accepting the package_name in package context makes sense. So maybe the build method should be called delete_by_id. We could maybe also consider just stating that 'build name' = str(build.id). This is something for you to consider.

>>> package_proxy.edit('@copr', 'copr', 'copr-backend')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: edit() missing 2 required positional arguments: 'source_type_text' and 'source_dict'

source_type_text and source_dict should not be required. I might e.g. want to change something in source_dict and keep source_type_text untouched. In that case, I would like not to need to define source_type_text at all.

>>> package_proxy.edit('@copr', 'copr', 'copr-backend', source_type_text='scm', source_dict={})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/clime/copr/python/copr/client_v3/proxies/package.py", line 87, in edit
    response = request.send()
  File "/home/clime/copr/python/copr/client_v3/requests.py", line 50, in send
    handle_errors(response.json())
  File "/home/clime/copr/python/copr/client_v3/requests.py", line 102, in handle_errors
    raise CoprRequestException(response_json["error"])
copr.client_v3.exceptions.CoprRequestException: {'clone_url': ['This field is required.'], 'scm_type': ['Not a valid choice'], 'srpm_build_method': ['This field is required.']}

The traceback is looking good. But scm_type, nor srpm_build_method should not be required. Both have default values per our current interface (scm_type is 'git', srpm_build_method is 'rpkg'). Note that we might want to change srpm_build_method to more generic source_build_method but that will be part of polishing the response interface.

>>> package_proxy.edit('@copr', 'copr', 'copr-backend', source_type_text='scm', source_dict={'clone_url': 'https://pagure.io/rpkg-util', 'scm_type': 'git', 'srpm_build_method': 'rpkg'})
Munch({'copr': 'copr', 'enable_net': False, 'id': 30744, 'name': 'copr-backend', 'old_status': None, 'owner': '@copr', 'source': {'clone_url': 'https://pagure.io/rpkg-util',

>>> package_proxy.add('@copr', 'copr', 'copr-backend', source_type_text='scm', source_dict={'clone_url': 'https://pagure.io/rpkg-util', 'scm_type': 'git', 'srpm_build_method': 'rpkg'})
Traceback (most recent call last):
copr.client_v3.exceptions.CoprRequestException: Package copr-backend already exists in copr @copr/copr.

Good.

>>> package_proxy.add('@copr', 'copr', 'copr-backend2', source_type_text='scm', source_dict={'clone_url': 'https://pagure.io/rpkg-util', 'scm_type': 'git', 'srpm_build_method': 'rpkg'})
Munch({'copr': 'copr', 'enable_net': False, 'id': 297135, 'name': 'copr-backend2', 'old_status': None, 'owner': '@copr', 'source': {'clone_url': 'https://pagure.io/rpkg-util'

OK.

>>> package_proxy.build('@copr', 'copr', 'copr-backend2')
Munch({'built_pkgs': None, 'chroots': {'epel-7-ppc64le': 'waiting', 'epel-7-x86_64': 'waiting', 'fedora-26-ppc64le': 'waiting', 'fedora-26-x86_64': 'waiting', 'fedora-27-ppc6

OK.

>>> package_proxy.reset('@copr', 'copr', 'copr-backend2')
Munch({'copr': 'copr', 'enable_net': False, 'id': 297135, 'name': 'copr-backend2', 'old_status': None, 'owner': '@copr', 'source': {}, 'source_type': 'unset', 'webhook_rebuild': False, '__response__': <copr.client_v3.requests.Response object at 0x7f71d03b96d8>})

source_type in the returned munch is string: 'unset'. Why do we then need to even have source_type_text? Could we handle source_type as a string everywhere?

Also there is source in the returned response whereas the edit method has source_dict as a param. It would be good to have those named the same way.

>>> package_proxy.edit('@copr', 'copr', 'copr-backend', source_type_text='scm', source_dict=Munch(clone_url='https://pagure.io/rpkg-util', scm_type='git', srpm_build_method='tito', committish='master'))
Munch({'copr': 'copr', 'enable_net': False, 'id': 30744, 'name': 'copr-backend', 'old_status': None, 'owner': '@copr', 'source': {'clone_url': 'https://pagure.io/rpkg-util', 'committish': 'master', 'spec': '', 'srpm_build_method': 'tito', 'subdirectory': '', 'type': 'git'}, 'source_type': 'scm', 'webhook_rebuild': False, '__response__': <copr.client_v3.requests.Response object at 0x7f71cdfc0da0>})

>>> package_proxy.edit('@copr', 'copr', 'copr-backend', source_type_text='scm', source_dict=Munch(clone_url='https://pagure.io/rpkg-util', scm_type='git', srpm_build_method='tito'))
Munch({'copr': 'copr', 'enable_net': False, 'id': 30744, 'name': 'copr-backend', 'old_status': None, 'owner': '@copr', 'source': {'clone_url': 'https://pagure.io/rpkg-util', 'committish': '', 'spec': '', 'srpm_build_method': 'tito', 'subdirectory': '', 'type': 'git'}, 'source_type': 'scm', 'webhook_rebuild': False, '__response__': <copr.client_v3.requests.Response object at 0x7f71d03b9780>})

Here, we can see that in the first invocation committish=master was set, in the second, it wasn't specified and it got unset. This is not a problem here but we will need to adjust the copr-cli interface because there the package attributes are specified by command-line arguments and if you e.g. do not specify scm_type, it shouldn't get redefined from 'svn' to 'git'. We will need to check this as part of copr-cli rewriting.

>>> package_proxy.edit('@copr', 'copr', 'copr-backend', source_type_text='custom')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: edit() missing 1 required positional argument: 'source_dict'

I think 'source_dict' should not be required here. Probably, empty dict should be passed to frontend, and frontend should use some default values defined to create the custom package and probably complain that some required argument has not been set.

What I am missing here is an option to get builds of a package. Either we should provide get_build_list to the package_proxy interface or we need to cover this in the build_proxy interface by enabling user to get build list for a given package name.

Moving on ProjectProxy now:

>>> project_proxy.search(query='@copr/copr')
[Munch({'additional_repos': 'https://copr-be.cloud.fedoraproject.org/results/%40modularity/modulemd/fedora-$releasever-$basearch/', 'auto_createrepo': True, 'auto_prune': Tru
...

Nice.

>>> project_proxy.get_list()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: get_list() missing 1 required positional argument: 'ownername'

Not a big deal, but do we need to require ownername?

>>> len(project_proxy.get_list('@copr'))
12

OK.

>>> project_proxy.edit('@copr', 'copr', auto_createrepo=False)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: edit() got an unexpected keyword argument 'auto_createrepo'

>>> project_proxy.edit('@copr', 'copr', {'auto_createrepo':False})
Traceback (most recent call last):
    ...
    return self.scan_once(s, idx=_w(s, idx).end())
simplejson.scanner.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

>>> project_proxy.edit('@copr', 'copr', Munch(auto_createrepo=False))
Traceback (most recent call last):
  ...
  File "/usr/lib64/python3.6/site-packages/simplejson/decoder.py", line 404, in raw_decode
    return self.scan_once(s, idx=_w(s, idx).end())

Not OK. I am not sure how I can edit a project settings.

>>> project_proxy.add('@copr', 'copr3', chroots=['fedora-26-x8_64'])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/clime/copr/python/copr/client_v3/proxies/project.py", line 97, in add
    response = request.send()
  File "/home/clime/copr/python/copr/client_v3/requests.py", line 50, in send
    handle_errors(response.json())
  File "/home/clime/copr/python/copr/client_v3/requests.py", line 102, in handle_errors
    raise CoprRequestException(response_json["error"])
copr.client_v3.exceptions.CoprRequestException: {'chroots': ['At least one chroot must be selected']}

Not sure why I am getting the 'At least one chroot must be selected' when I actually selected a chroot.

>>> project_proxy.add('@copr', 'copr3', chroots=['fedora-28-x8_64', 'fedora-27-x86_64'], additional_repos=[])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: add() got an unexpected keyword argument 'additional_repos'

Not OK.

>>> project_proxy.fork(ownername='@copr', projectname='copr', dstownername='@copr', dstprojectname='copr-fork')
Munch({'additional_repos': '', 'auto_createrepo': True, 'auto_prune': True, 'description': 'copr repository  with updated dnf-plugins-core to support copr-prune-repo.py', 'full_name': 'clime/copr-fork', 'instructions': '', 'last_modified': None, 'name': 'copr-fork', 'owner': 'clime', 'persistent': False, 'unlisted_on_hp': False, 'use_bootstrap_co

OK.

Next...ProjectChrootProxy:

>>> project_chroot_proxy.get('@copr', 'copr', 'fedora-27-x86_64')
Munch({'buildroot_pkgs': None, 'comps_name': None, 'copr_id': 5893, 'mock_chroot': 'fedora-27-x86_64', 'repos': '', '__response__': <copr.client_v3.requests.Response object at 0x7f71d03c5080>})

OK.

>>> project_chroot_proxy.edit('@copr', 'copr', 'fedora-27-x86_64', repos='https://foo/bar https://x/y')
Traceback (most recent call last):
    raise CoprRequestException(response_json["error"])
copr.client_v3.exceptions.CoprRequestException: {'repos': ["A list of http[s] URLs separated by whitespace characters is needed ('h' doesn't seem to be a valid URL)."]}

The error says "A list of http[s] URLs separated by whitespace characters is needed" but 'https://foo/bar https://x/y' is not accepted.

>>> project_chroot_proxy.edit('@copr', 'copr', 'fedora-27-x86_64', repos=['https://foo/bar', 'https://bar/baz'])
Munch({'buildroot_pkgs': '', 'comps_name': None, 'copr_id': 5893, 'mock_chroot': 'fedora-27-x86_64', 'repos': 'https://foo/bar https://bar/baz', '__response__': <copr.client_v3.requests.Response object at 0x7f71cdfcb668>})

This works but the repos list gets transformed into string separated by space, would it be possible to keep this as list of strings?

>>> project_chroot_proxy.edit('@copr', 'copr', 'fedora-27-x86_64')
Munch({'buildroot_pkgs': '', 'comps_name': None, 'copr_id': 5893, 'mock_chroot': 'fedora-27-x86_64', 'repos': '', '__response__': <copr.client_v3.requests.Response object at 0x7f71cdfcb080>})

This is not ok. The repos param was not specified and as a result it became unset (set to empty string) in the resulting definition. That's not good. We need to be especially careful about this. If param is not specified for edit methods, the value should not get changed. Unit tests for this would be good as well.

>>> project_chroot_proxy.get_build_config('@copr', 'copr', 'fedora-27-x86_64')
Munch({'additional_packages': [], 'chroot': 'fedora-27-x86_64', 'project_id': 'group_copr-copr', 'repos': [{'id': 'copr_base', 'name': 'Copr repository', 'url': 'http://localhost:5002/results/@copr/copr/fedora-27-x86_64/'}], 'use_bootstrap_container': False, '__response__': <copr.client_v3.requests.Response object at 0x7f71d03c5828>})

This method seems very similar to:

>>> project_chroot_proxy.get('@copr', 'copr', 'fedora-27-x86_64')
Munch({'buildroot_pkgs': '', 'comps_name': None, 'copr_id': 5893, 'mock_chroot': 'fedora-27-x86_64', 'repos': '', '__response__': <copr.client_v3.requests.Response object at 0x7f71cdfd3be0>})

But the first one has the setting inherited from project...this seems to be OK.

Next ModuleProxy:

>>> module_proxy.build_from_url('@copr', 'copr', 'https://src.fedoraproject.org/modules/testmodule/raw/master/f/testmodule.yaml')
Munch({'nsv': 'testmodule-master-20180510150906', '__response__': <copr.client_v3.requests.Response object at 0x7f71d03ab7f0>})

OK.

After downloading the testmodule.yaml locally:

>>> module_proxy.build_from_file('@copr', 'copr', 'testmodule.yaml')
Munch({'nsv': 'testmodule-master-20180510151239', '__response__': <copr.client_v3.requests.Response object at 0x7f71cdfd37b8>})

OK.

Finally coming...Client:

>>> from copr import Client
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: cannot import name 'Client'

I think this should be possible (and if possible then the only option).

>>> from copr.client_v3 import Client
>>> client = Client(Munch({'username': 'clime', 'login': 'ppoonybxsbhqldybrkrd', 'token': 'wbnsbwdvtbrtfzwsfvplqusqbynkaa', 'copr_url': 'http://localhost:8080'}))
>>> client.
client.build_proxy               client.config                    client.create_from_config_file(  client.package_proxy

The list comes from tab auto-completion. I can see that only build_proxy and package_proxy were added. Could we possibly add other proxies as well?

There is client.create_from_config_file but I don't think that method is necessary (it should be a class method, not instance method also). Instead we should either provide a helper method to create 'config' from a copr config file or perhaps better Proxy.from_file() methods and then also Client.from_file(). create_from_config_file seems a bit verbose.

Other notes:

>>> from copr.client_v3 import helpers
>>> helpers.
helpers.List(     helpers.refresh(

I really wonder now what the refresh method should be good for. Ideally here, I would like to know what the method does already, meaning that the naming is probably not sufficient.

So, let's examine the whole namespace:

>>> import copr.client_v3
>>> copr.client_v3.
copr.client_v3.BuildProxy(               copr.client_v3.GeneralProxy(             copr.client_v3.ProjectProxy(             copr.client_v3.exceptions
copr.client_v3.Client(                   copr.client_v3.ModuleProxy(              copr.client_v3.Request(                  copr.client_v3.helpers
copr.client_v3.CoprRequestException(     copr.client_v3.POST                      copr.client_v3.Response(                 copr.client_v3.proxies
copr.client_v3.CoprValidationException(  copr.client_v3.PackageProxy(             copr.client_v3.absolute_import           copr.client_v3.refresh(
copr.client_v3.GET                       copr.client_v3.ProjectChrootProxy(       copr.client_v3.client                    copr.client_v3.requests

I wonder about copr.client_v3.client. The whole client_v3 namespace does not need to be there, I think but .client seems to be also unnecessary. Not sure copr.client_v3.GET and copr.client_v3.POST need to be there but that's just really minor note.

I am quite delighted about the API interface as a user. There are some bugs though that should be fixed.

Thank you for the review @clime, it is very helpful.

commented out the assert in test_requests.py

Fixed

Why the build_proxy does not have get_list as well?

Done

simplejson.scanner.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

Fixed in 15e29ce7

I wonder...what about having ownername and projectname part of **kwargs and merge them with config. Meaning, that you could put ownername and projectname into the config param and then not need to specify them when creating the build? What do you think about it?

Interesting idea, I will comment it later

I wasn't able to start PyPI build for both python versions at once.

Fixed

build_proxy.create_from_rubygems('A_123')
Again, it would be nice if this just worked.

You are missing an ownername and projectname. Let's disscuss it below

build_proxy.create_from_custom('@copr', 'copr', script='echo foo')
There seems to be some problem with the chroot param.

Fixed

We should be thinking about removing 'old_status' there. Not necessarily in this PR but we should polish the returned results so that they always make sense before the new API is published.

Ok, let's do it later, in smaller PR. So we can really focus on the results that we return

Proxy should be actually an abstract class (interface) defining the interface for the individual concrete proxy classes (BuildProxy, PackageProxy,...). In that class, there should be something like:

We currently have BaseProxy with few methods, so we can put the interface methods in there, or we can create also ProxyInterface and let either BaseProxy or its descendants to inherit from it.

build_proxy.delete method accepts id as an argument. We should try to unify this if possible. Consider calling this method delete_by_name, accepting a single param name (if ownername, projectname are predefined):
So if we want to delete by ids and by names as well, we likely need to separate the methods.

It would be good to have the class Proxy with all the base methods explicitly defined if it is not yet.

Looking further at the PackageProxy methods like get or edit, they also use package_name as a param so delete accepting the package_name in package context makes sense. So maybe the build method should be called delete_by_id. We could maybe also consider just stating that 'build name' = str(build.id). This is something for you to consider.

Yes, that is a good point. I don't like e.g. delete_by_name because everything always works with some kind of name, which is natural identifier for such resource. Only builds are "naturally identified" by ID. But it could be nice, if we allow querying everything also through ID. In that case, I would add methods get_by_id.

source_type_text and source_dict should not be required. I might e.g. want to change something in source_dict and keep source_type_text untouched. In that case, I would like not to need to define source_type_text at all.

Fixed, although for this use-case only source_type_text needs to be optional.

The traceback is looking good. But scm_type, nor srpm_build_method should not be required. Both have default values per our current interface (scm_type is 'git', srpm_build_method is 'rpkg'). Note that we might want to change srpm_build_method to more generic source_build_method but that will be part of polishing the response interface.

Not fixed yet

source_type in the returned munch is string: 'unset'. Why do we then need to even have source_type_text? Could we handle source_type as a string everywhere?

Also there is source in the returned response whereas the edit method has source_dict as a param. It would be good to have those named the same way.

Renamed source_type_text to source_type and source_dict to source

Here, we can see that in the first invocation committish=master was set, in the second, it wasn't specified and it got unset. This is not a problem here but we will need to adjust the copr-cli interface because there the package attributes are specified by command-line arguments and if you e.g. do not specify scm_type, it shouldn't get redefined from 'svn' to 'git'. We will need to check this as part of copr-cli rewriting.

I think, that it is a same issue that project_chroot_proxy.edit has, so I probably should fix it here, not in CLI.

package_proxy.edit('@copr', 'copr', 'copr-backend', source_type_text='custom')
I think 'source_dict' should not be required here. Probably, empty dict should be passed to frontend, and frontend should use some default values defined to create the custom package and probably complain that some required argument has not been set.

Fixed ... well, partially. It is not required to specify source_dict (now renamed to just source), but this particular command will fail, because there is no default value for script field.

What I am missing here is an option to get builds of a package. Either we should provide get_build_list to the package_proxy interface or we need to cover this in the build_proxy interface by enabling user to get build list for a given package name.

Good point. I prefer modifying BuildProxy.get_list and add an optional parameter package= to it. What do you think?

Not a big deal, but do we need to require ownername?

I would like to have either ownername and projectname required everywhere, or have them everywhere as optional. I will write more about it a bit later.

Not OK. I am not sure how I can edit a project settings.

You were doing it right, you just set an unexpected argument.

project_proxy.edit('@copr', 'copr', auto_createrepo=False)

Is the auto_createrepo meant to be disable_createrepo from APIv1?

Not sure why I am getting the 'At least one chroot must be selected' when I actually selected a chroot.

Because you have a typo in the chroot name 'fedora-26-x8_64'. The message text is kinda bad, but it comes from the validation, that has nothing to do with the new API, so I would prefer to not change it in this particular PR.

TypeError: add() got an unexpected keyword argument 'additional_repos'
Not OK.

The additional_repos parameter was renamed to just repos

This works but the repos list gets transformed into string separated by space, would it be possible to keep this as list of strings?

Fixed

This is not ok. The repos param was not specified and as a result it became unset (set to empty string) in the resulting definition. That's not good. We need to be especially careful about this. If param is not specified for edit methods, the value should not get changed. Unit tests for this would be good as well.

Agreed, that was a bed behavior. Fixed.

This method seems very similar to:
But the first one has the setting inherited from project...this seems to be OK.

Well, we can throw away one of them if we want. They were both in the APIv1, but it is possible that one of them is useless

Finally coming...Client:

I will get to the client a bit later.

The list comes from tab auto-completion. I can see that only build_proxy and package_proxy were added. Could we possibly add other proxies as well?

Sure, fixed

There is client.create_from_config_file but I don't think that method is necessary (it should be a class method, not instance method also). Instead we should either provide a helper method to create 'config' from a copr config file or perhaps better Proxy.from_file() methods and then also Client.from_file(). create_from_config_file seems a bit verbose.

I am a fan of "Instead we should either provide a helper method to create 'config' from a copr config file". It should be there for sure. The question is, whether to add also Client.from_file() and Proxy.from_file() or rather let user load the config in one separate step and then supply it to the proxy/client. I would prefer it this way, but the annoying part is that users will need to import one more thing just for loading a config in 90% cases.

I really wonder now what the refresh method should be good for. Ideally here, I would like to know what the method does already, meaning that the naming is probably not sufficient.

The refresh method takes some result munch, finds out the original request, that was sent to obtain that munch, sends it again and updates the munch values. It is not neccessary at all, you can always call a get method, to get new munch. But I can imagine, that someone could prefer the refresh method over it.

The idea behind it was to make something like this possible

build = client.build_proxy.create_from_url("@copr", "foo", "http://foo.ex/bar.src.rpm")
while build.status not in ["failed", "canceled", "succeeded"]:
    time.sleep(10)
    # either
    build = client.build_proxy.get(build.id)
    # or
    refresh(build)
print("Build finished")

But now I realize that the refresh logic woud have to be different, that it is now. Because now, this particular example will as a side-effect submit a new build on every refresh call. We can fix the behavior or drop it completely. What is your opinion on it?

Not sure copr.client_v3.GET and copr.client_v3.POST need to be there but that's just really minor note.

You are right, they don't. They are used only internally. Fixed in 7b0b1eef

I wonder about copr.client_v3.client. The whole client_v3 namespace does not need to be there, I think but .client seems to be also unnecessary.
why are Proxies accessible through the client module only? client module, in case we really want it, should only supply the Client class, an instance of which aggregates proxies for easier access to them. It's the same e.g. here:

from copr.client_v3 import BuildProxy, PackageProxy
from copr.pagination import Pagination

This is an interesting topic. One one hand, it looks better, it is shorter, user can type it faster. Also for a new user, it makes more sense than the version, I implemented. On the other hand it will create a mess in the versions of API that we have. Meaning that every time I import something directly from copr package, I need to check in the documentation whether it is a part of APIv3 or some leftover from previous API. This will be solved in some time, once previous API code is removed. It may also complicate creating a hypothetical APIv4 (exaggerated scenario - we all disappear from the world, new guys takes over the Copr, totally dislike this API and create a new one).

I am all for renaming the copr.client_v3 namespace, to just copr.v3. It will solve the confusion with "client" word. Than, if we really want to provide things directly through copr, we can import * from copr.v3 and provide it. What do you think?

I wonder...what about having ownername and projectname part of **kwargs and merge them with config. Meaning, that you could put ownername and projectname into the config param and then not need to specify them when creating the build? What do you think about it?

I like the idea of not having to specify ownername and projectname over and over again, but I am not sure how to do it.

Having them specified in the config imho abuses it. Currently, the idea is that config = representation of your config file (e.g. ~/.config/copr) = your auth information. Adding ownername and projectname will break this definitnion. Which alone, doesn't necesarily have to be a problem, but we wouldn't allow to specify it in the config file, so user will have to read the config file and then append the project info into to it. It is too much complicated imho.

Another possibility would be to add ownername=None and projectname=None parameters to the Proxy* and Client init methods. This would help very much, when user wants to work with just one project. Otherwise, he would have to create a proxy/client for each project, that he works with, or specify ownername and projectname in the method call. Which is alright. What really frustrates me about it, is that now we have ownername and projectname params non-optional, and they are always first two positional parameters, always in same order (consistency). If we make them optional, it will imho create a lot of mess in cases, that user doesn't want to supply the project information directly to proxy/client. Thoughts? Am I beeing too purist? I see the APIv1 as deterrent example, where projectname is usually the first parameter and the username is on random position among the optional parameters.

Renamed source_type_text to source_type and source_dict to source

So, first, source_dict is probably a better name for the var than source because it says much more about the expected value. source_type instead of just source_type_text is ok.

But...note, that you should think in the overall code context. Can we do the changes everywhere in the code? Because, I would prefer using the same variables, we use internally. We do not need to do it in this PR but it would be nice if you had a look how difficult it is to do those renames everywhere.

I would really prefer source_dict and source_type. source by itself is really not saying much about the variable.

Good point. I prefer modifying BuildProxy.get_list and add an optional parameter package= to it. >What do you think?

I think, we could offer both methods. BuildProxy.get_list with package param in minimum.

Because you have a typo in the chroot name 'fedora-26-x8_64'. The message text is kinda bad, but it comes from the validation, that has nothing to do with the new API,

Well, the message is being shown in the API. So it would be good to fix it too (I agree we can keep this for later).

The additional_repos parameter was renamed to just repos

Note that we want to have those params called everywhere in the code the same way. It does not need to be this way immediately but should. So go through the code and pick the variable name that's most suitable (repos is good if it includes really all the enabled repos for the build including the copr's own repo).

Well, we can throw away one of them if we want. They were both in the APIv1, but it is possible that one of them is useless

This was about project_chroot_proxy.get_build_config and project_chroot_proxy.get. Well check if get_build_config is being used anywhere. But I don't mind keeping the method. It might be useful to be easily get the build configuration of the chroot merged with project. But, please, check how this merging works and if it makes sense to you.

Having them specified in the config imho abuses it. Currently, the idea is that config = representation of your config file (e.g. ~/.config/copr) = your auth information. Adding ownername and projectname will break this definitnion. Which alone, doesn't necesarily have to be a problem, but we wouldn't allow to specify it in the config file, so user will have to read the config file and then append the project info into to it. It is too much complicated imho.

Ye, well, I wouldn't mind putting them into the config file too as optional params. The config doesn't need to be just about auth information. The truth is that it would complicate copy-pasting from https://copr.fedorainfracloud.org/api/. I am not sure if this is an actual problem. I would say not, user can just replace the beginning and keep the rest of the lines, I'd say. The question here such setting (e.g. default ownername/projectname) would be also useful in copr-cli, e.g. to be able to write only copr-cli build <srpm_path>. Btw note that we now probably have also a chance to change the copr-cli interface, maybe not immediately but we could talk about it after we finish this.

Another possibility would be to add ownername=None and projectname=None parameters to the Proxy* and Client init methods. This would help very much, when user wants to work with just one project. Otherwise, he would have to create a proxy/client for each project, that he works with, or specify ownername and projectname in the method call. Which is alright. What really frustrates me about it, is that now we have ownername and projectname params non-optional, and they are always first two positional parameters, always in same order (consistency). If we make them optional, it will imho create a lot of mess in cases, that user doesn't want to supply the project information directly to proxy/client. Thoughts? Am I beeing too purist? I see the APIv1 as deterrent example, where projectname is usually the first parameter and the username is on random position among the optional parameters.

Adding it to proxy as optional params might be also ok but then there is no way to pass (have) that configuration to copr-cli if we wanted to.

Apart from saving user from some typing, the idea here is that, you would be able to write:

package_proxy.get_list(ownername='@copr', projectname='copr') - get just packages for @copr/copr
package_proxy.get_list(ownername='@copr')  - get all packages for @copr group
package_proxy.get_list() - get all packages in Copr

Maybe this is not necessary to have. The current variant (with the positional params) is pretty solid so I don't mind just keeping in case we can't figure out something that would be solid too and it would bring some benefits.

The traceback is looking good. But scm_type, nor srpm_build_method should not be required. Both have default values per our current interface (scm_type is 'git', srpm_build_method is 'rpkg'). Note that we might want to change srpm_build_method to more generic source_build_method but that will be part of polishing the response interface.

Not fixed yet

Fixed

But now I realize that the refresh logic woud have to be different, that it is now. Because now, this particular example will as a side-effect submit a new build on every refresh call. We can fix the behavior or drop it completely. What is your opinion on it?

I've removed it, as it is just a syntactical sugar (and currently implemented wrong)

I would really prefer source_dict and source_type. source by itself is really not saying much about the variable.

Np, source_dict it is

I think, we could offer both methods. BuildProxy.get_list with package param in minimum.

I've implemented the BuildProxy.get_list(..., package=None) option. I believe that we won't need the other method, but we can implement it in the future if I am wrong.

I am a fan of "Instead we should either provide a helper method to create 'config' from a copr config file". It should be there for sure. The question is, whether to add also Client.from_file() and Proxy.from_file() or rather let user load the config in one separate step and then supply it to the proxy/client. I would prefer it this way, but the annoying part is that users will need to import one more thing just for loading a config in 90% cases.

So far, I've added only independent config_from_file(path=None) method. We can easily add something like Client.from_file() and Proxy.from_file() in the future, but I am not sure about it, so let's just wait.

I am all for renaming the copr.client_v3 namespace, to just copr.v3. It will solve the confusion with "client" word. Than, if we really want to provide things directly through copr, we can import * from copr.v3 and provide it. What do you think?

I've renamed it.

@frostyx: what's the status here?

PR#278 is merged and there is a linked blog post on https://copr.fedorainfracloud.org/ announcing a new API. I think, that we can safely close this issue.

Currently, there is a WIP copr-cli rewrite to use APIv3 and a blog post regarding migration from APIv1, so we can also wait and close this issue once those are finished.

PR#303 with copr-cli rewrite is also merged, so I think that we can safely close this.