#1322 cli, frontend, python: enable deleting multiple builds from cli
Merged 3 years ago by praiskup. Opened 3 years ago by dturecek.
copr/ dturecek/copr delete-multiple-builds  into  master

file modified
+4 -5
@@ -500,8 +500,8 @@ 

          self._watch_builds(args.build_id)

  

      def action_delete_build(self, args):

-         build = self.client.build_proxy.delete(args.build_id)

-         print("Build deleted")

+         result = self.client.build_proxy.delete_list(args.build_id)

+         print("Build(s) {0} were deleted.".format(", ".join(map(str, result["builds"]))))

  

      #########################################################

      ###                   Chroot actions                  ###
@@ -1039,11 +1039,10 @@ 

  

      # create the parser for the "delete-build" command

      parser_delete = subparsers.add_parser("delete-build",

-                                          help="Delete build specified by its ID")

-     parser_delete.add_argument("build_id", help="Build ID", type=int)

+                                           help="Delete builds specified by their IDs")

+     parser_delete.add_argument("build_id", help="Build ID", type=int, nargs="+")

      parser_delete.set_defaults(func="action_delete_build")

  

- 

      #########################################################

      ###                   Chroot options                  ###

      #########################################################

@@ -25,6 +25,7 @@ 

  from coprs import helpers

  from coprs.exceptions import (

      ActionInProgressException,

+     BadRequest,

      ConflictingRequest,

      DuplicateException,

      InsufficientRightsException,
@@ -960,15 +961,43 @@ 

          db.session.delete(build)

  

      @classmethod

-     def delete_multiple_builds(cls, user, builds):

+     def delete_builds(cls, user, build_ids):

          """

+         Delete builds specified by list of IDs

+ 

          :type user: models.User

-         :type builds: list of models.Build

+         :type build_ids: list of Int

          """

          to_delete = []

+         no_permission = []

+         still_running = []

+ 

+         build_ids = set(build_ids)

+         builds = cls.get_by_ids(build_ids)

          for build in builds:

-             cls.check_build_to_delete(user, build)

-             to_delete.append(build)

+             try:

+                 cls.check_build_to_delete(user, build)

+                 to_delete.append(build)

+             except InsufficientRightsException:

+                 no_permission.append(build.id)

+             except ActionInProgressException:

+                 still_running.append(build.id)

+             finally:

+                 build_ids.remove(build.id)

+ 

+         if build_ids or no_permission or still_running:

+             msg = ""

+             if no_permission:

+                 msg += "You don't have permissions to delete build(s) {0}.\n"\

+                     .format(", ".join(map(str, no_permission)))

+             if still_running:

+                 msg += "Build(s) {0} are still running.\n"\

+                     .format(", ".join(map(str, still_running)))

+             if build_ids:

+                 msg += "Build(s) {0} don't exist.\n"\

+                     .format(", ".join(map(str, build_ids)))

+ 

+             raise BadRequest(msg)

  

          if to_delete:

              ActionsLogic.send_delete_multiple_builds(to_delete)

@@ -191,9 +191,9 @@ 

  

          to_delete = []

          for build in package.builds:

-             to_delete.append(build)

+             to_delete.append(build.id)

  

-         builds_logic.BuildsLogic.delete_multiple_builds(user, to_delete)

+         builds_logic.BuildsLogic.delete_builds(user, to_delete)

          db.session.delete(package)

  

  

@@ -281,3 +281,15 @@ 

      BuildsLogic.delete_build(flask.g.user, build)

      db.session.commit()

      return flask.jsonify(build_dict)

+ 

+ 

+ @apiv3_ns.route("/build/delete/list", methods=POST)

+ @api_login_required

+ def delete_builds():

+     """

+     Delete builds specified by a list of IDs.

+     """

+     build_ids = flask.request.json["builds"]

+     BuildsLogic.delete_builds(flask.g.user, build_ids)

+     db.session.commit()

+     return flask.jsonify({"builds": build_ids})

@@ -17,7 +17,8 @@ 

  

  from coprs.exceptions import (ActionInProgressException,

                                InsufficientRightsException,

-                               UnrepeatableBuildException)

+                               UnrepeatableBuildException,

+                               BadRequest)

  

  

  @coprs_ns.route("/build/<int:build_id>/")
@@ -479,12 +480,15 @@ 

  

      to_delete = []

      for build_id in build_ids:

-         build = ComplexLogic.get_build_safe(build_id)

-         to_delete.append(build)

+         to_delete.append(int(build_id))

  

-     builds_logic.BuildsLogic.delete_multiple_builds(flask.g.user, to_delete)

+     try:

+         builds_logic.BuildsLogic.delete_builds(flask.g.user, to_delete)

  

-     db.session.commit()

-     build_ids_str = ", ".join(build_ids).strip(", ")

-     flask.flash("Builds {} have been deleted successfully.".format(build_ids_str), "success")

-     return flask.jsonify({"msg": "success"})

+         db.session.commit()

+         build_ids_str = ", ".join(build_ids).strip(", ")

+         flask.flash("Builds {} have been deleted successfully.".format(build_ids_str), "success")

+         return flask.jsonify({"msg": "success"})

+     except BadRequest as e:

+         flask.flash(e, "error")

+         return flask.jsonify({"msg": "error"})

@@ -9,7 +9,8 @@ 

  from coprs import app

  

  from copr_common.enums import StatusEnum

- from coprs.exceptions import ActionInProgressException, InsufficientRightsException, MalformedArgumentException

+ from coprs.exceptions import ActionInProgressException, InsufficientRightsException, \

+                              MalformedArgumentException, BadRequest

  from coprs.logic.actions_logic import ActionsLogic

  from coprs.logic.builds_logic import BuildsLogic

  
@@ -248,8 +249,7 @@ 

          self.db.session.commit()

  

          assert len(ActionsLogic.get_many().all()) == 0

-         BuildsLogic.delete_multiple_builds(self.u1, [self.b1, self.b2,

-                                                      self.b_pr])

+         BuildsLogic.delete_builds(self.u1, [self.b1.id, self.b2.id, self.b_pr.id])

          self.db.session.commit()

  

          assert len(ActionsLogic.get_many().all()) == 1
@@ -300,6 +300,39 @@ 

          with pytest.raises(NoResultFound):

              BuildsLogic.get(self.b1.id).one()

  

+     @pytest.mark.usefixtures("f_users", "f_coprs", "f_builds")

+     def test_delete_multiple_builds(self):

+         """

+         Test deleting multiple builds at once.

+         """

+         self.b4_bc[0].status = StatusEnum("succeeded")

+         build_ids = [self.b1.id, self.b2.id, self.b3.id, self.b4.id, 1234]

+         with pytest.raises(BadRequest) as err_msg:

+             BuildsLogic.delete_builds(self.u2, build_ids)

+ 

+         msg1 = "Build(s) {0} are still running".format(self.b3.id)

+         msg2 = "Build(s) 1234 don't exist"

+         msg3 = ("You don't have permissions to delete build(s) {0}, {1}"

+                 .format(self.b1.id, self.b2.id))

+ 

+         for msg in [msg1, msg2, msg3]:

+             assert msg in str(err_msg.value)

+ 

+         for build_id in [self.b1.id, self.b2.id, self.b3.id, self.b4.id]:

+             BuildsLogic.get(build_id).one()

+ 

+         self.c1.user = self.u2

+         build_ids = [self.b1.id, self.b4.id]

+         BuildsLogic.delete_builds(self.u2, build_ids)

+         self.db.session.commit()

+ 

+         assert len(ActionsLogic.get_many().all()) == 1

+ 

+         with pytest.raises(NoResultFound):

+             BuildsLogic.get(self.b1.id).one()

+         with pytest.raises(NoResultFound):

+             BuildsLogic.get(self.b4.id).one()

+ 

      def test_mark_as_failed(self, f_users, f_coprs, f_mock_chroots, f_builds):

          self.b1.source_status = StatusEnum("succeeded")

          self.db.session.commit()

@@ -275,5 +275,17 @@ 

          """

          endpoint = "/build/delete/{0}".format(build_id)

          request = Request(endpoint, api_base_url=self.api_base_url, method=POST, auth=self.auth)

+ 

+     def delete_list(self, build_ids):

+         """

+         Delete multiple builds specified by a list of their ids.

+ 

+         :param list[int] build_id:

+         :return: Munch

+         """

+ 

+         endpoint = "/build/delete/list"

+         data = {"builds": build_ids}

+         request = Request(endpoint, data=data, api_base_url=self.api_base_url, method=POST, auth=self.auth)

          response = request.send()

          return munchify(response)

no initial comment

Metadata Update from @dturecek:
- Pull-request tagged with: needs-work

3 years ago

rebased onto 1a2b721ce1d64c11c969471dc4107f2f35480398

3 years ago

I fixed the issues I mentioned on the meeting, I think this is ready for review now.

Metadata Update from @dturecek:
- Pull-request untagged with: needs-work

3 years ago

I like the precise output here, saying the info ... but I'd much rather see some
transactional behavior which would remove everything, or nothing (and kept
this nice stat output).

Maybe it is a possibility to return the stats here, in some structured dict. And provide the nice output even in web-UI?

+1 for the transactional behavior, either it is possible to delete everything and commit or failure.

Also, I would try to avoid such complicated msg handing. Please raise an appropriate exception on frontend, make sure python-copr reacts on it by throwing its own exception and here just print its text.

For other multi-word endpoints, we use a dash as a delimiter, so I would go with /build/delete-multiple.

Or ... too bad our /build/delete endpoint wasn't originally implemented to accept a list of IDs, then deleting one build would be just a special case of it. Anyway, what about enhancing the existing endpoint to accept both one integer ID and list of IDs and based on input, it would call an appropriate function. WDYT @praiskup?

Yes, if we keep frontend compatible with old clients, sounds good.

Also, I would try to avoid such complicated msg handing. Please raise an appropriate exception on frontend, make sure python-copr reacts on it by throwing its own exception and here just print its text.

Yes, it makes sense. What I like about this is that user gets more detailed view
what issues were in the transaction (not just the first issue). That said, the exception
could hold some structure with more errors, not just one message. That should
allow us to both "pretty" print the exception messages to commandline, and web-UI.

That said, the exception could hold some structure with more errors, not just one message.

What additional information are we interested about? IIRC we currently return status code and the message, but there should be a possibility to add additional information.

I meant, the error can have array of messages, like:

- Build A, B, C ... D is still running.
- You don't have permissions for removing F, G builds.
- ... dunno ...

In web-ui, each message could be displayed as separate flash message, on commandline
you probably want to print each on its own line. Or dunno, maybe it is enough to wrap
it on newlines....? (array sounds nicer to me, but I don't have strong preference).

rebased onto d3a54c3c272458f31a9fb460527cb5481300f384

3 years ago

Thanks for the review, I've added the transactional behavior and moved the error message generation over to frontend. I'm still figuring out how to unify the endpoint with build/delete while keeping the compatibility with old clients.

1 new commit added

  • cli, frontend, python: unify delete-build and delete-builds commands
3 years ago

I've unified both endpoints. However, the way I did it seems a little ugly to me, so if you have a better idea on how to do this , please share.

I've unified both endpoints. However, the way I did it seems a little ugly to me, so if you have a better idea on how to do this , please share.

I agree. It is ugly, sorry for not seeing it before ...
What about to have /build/delete and /builds/delete? @frostyx?

Metadata Update from @praiskup:
- Pull-request tagged with: needs-work

3 years ago

rebased onto 56821a77f02886eab610ef6fbcf779b71168359d

3 years ago

I've changed it so that copr-cli now accesses the new endpoint /builds/delete regardless of the number of builds to delete.

This probably needs to stay compatible with previous API version, so we still need to even accept non-array argument.

I somewhat dislike the dont_check option. It would be way nicer to use this method
everywhere where we do batch removal requests. And always do the check.

What if we had BuildsLogic.delete_builds(user, build_ids), which would do all the checks (including object not found)?

We really shouldn't special case API here. This awesome error diagnosis should be used for web-UI!

rebased onto 2be31e7dd4f174c04b7bdfe87bffa7f8ee139f52

3 years ago

Please mention here what to use instead.

Still, doing only one query would be better; Please transform build_ids to set(), and do one query with in_() statement, and iterate over builds.
As you process the builds, drop the build_ids from the set ... and if some build id is left, it was not found.

This is very promising! Thank you for the update.

Please add some test-cases, and drop need-work tag once this is ready for review.

rebased onto fa39d10aac54ddf294646388741a3ab7dd73e193

3 years ago

rebased onto ee8fa25f151b2e589c7d466f3a7c384a7b72f3c3

3 years ago

I've addressed your comments.
Here's the screenshot.

Metadata Update from @dturecek:
- Pull-request untagged with: needs-work

3 years ago

I don't know why I was confused a bit in commandline ... perhaps because the (s)... I sort of expected the IDs to be printed .... it looked buggy, but it isn't.

What is in the build variable anyways? Can we use it to make the output a bit nicer? If not, please drop it.

I'd probably at least change the message to Delete request succeeded..

  • I'd prefer to have better commandline success output
  • please rebase, and fix alembic migration as new revisions are in master now

Otherwise LGTM. (I'd love to see updated beaker test for multi build delete through cmdline, but not a hard requirement here)

Ah, there's no alembic migration here! So only the cmdline output....

rebased onto 7f67f3141f8bf82eebf59912f36f542bc1a0fe6f

3 years ago

rebased onto 096cb3d4c58f6f42b208bbc7d419a7770c6fea7e

3 years ago

I've changed the command line success output to show a list of deleted builds.

@frostyx do you want to take a look at the api change?

I am not sure if we want to go with /builds/delete here. Everywhere else we use singular (e.g. /build/list/ instead of e.g. /builds).

If we wanted to be more consistent, I would go with something like /build/delete/list (which is definitely less pretty, I admit that).

It's Just my POV, I am not forcing this.

I am a bit afraid that if we ease the endpoint restrictions (for the lack of a better word), it will turn into a complete mess again. So far we still can e.g. create specific namespaces for our endpoints

@apiv3_builds_ns.route("/<int:build_id>/", methods=GET)

Just nitpicking, but please define endpoint first, no blank line after, and then data ... It's consistent across all functions in proxies.

Since we use get_list and not get_multiple I would vote for delete_list

Is there any reason why f_builds are not enough?

I am really sorry for the late review, I had it on my list for a long time, but I was swamped :-/

rebased onto 6db645c35ba01ead8514c5812b169ffb17d56bf8

3 years ago

Thanks for the review, I've changed the endpoint to build/delete/list and method delete_list as per your comments.

As for adding a fixture on top of f_builds - it has only one build that can be deleted by one user (well not really, u1 can delete a build of another user as he is an admin. But then we can't test the "no permission to delete" message for u1). I've tried adding additional builds to f_builds but that broke several other tests. Adding a new fixture seemed easier than fixing the tests :)

However, if you'd really prefer to not add another fixture, I guess I could fix them.

As for adding a fixture on top of f_builds - it has only one build that can be deleted by one user (well not really, u1 can delete a build of another user

Personally, I would change the owner of builds provided by f_builds within your test. I don't think it's messy or anything. And if you do it in a loop, it will take only 2-3 lines. Which, to me, is a
better option than introducing such fixture. You can even have the test in a separate class (in the same file) and having a setUp method for modifying the builds, if you don't want to do it in the test method.

But I guess it is a matter of opinion and I am not forcing mine, so ultimately it's up to you.
What do you think?

rebased onto f0d39edc485eee9b54f137e7591377bad6605036

3 years ago

I haven't thought of changing the owner. Thanks for the tip, I've changed the test to do the same thing without the need for a separate fixture.

I think this shouldn't be necessary and you want to go with @pytest.mark.usefixtures instead. See e.g.test_modules_logic:TestModulesLogic.test_get_by_nsv_str as an example.

Can you please rebase the branch? It seems that there is some merge conflict with master.
Also, please see the Jenkins errors and try to fix at least those that you find reasonable. You can reproduce them locally with make lint

rebased onto 99f437e3f82ad24aadc54eecf89662c6307aada7

3 years ago

I've fixed the merge conflict. As for the jenkins errors, they don't make much sense to me - they seem to concern mainly files I haven't even changed. The output I get when running make lint locally is different and I think it's mostly unrelated to the changes I've made (Like too many public methods or too many lines).

As for the jenkins errors, they don't make much sense to me - they seem to concern mainly files I haven't even changed.

I already reported this:
https://github.com/kdudka/csdiff/issues/8

The output I get when running make lint locally is different and I think it's mostly unrelated to the changes I've made

You would need newer csdiff package (it will take some time till it gets to fedora):
https://copr.fedorainfracloud.org/coprs/praiskup/csdiff-for-copr/

This is not correct hint anymore, and I'm curious whteher we want to deprecate the API method anyway...

whteher we want to deprecate the API method anyway...

True, it works just fine. Only problem with it is when people use it in a loop. And they did that because there was no better option, so I think we don't need to deprecate it.

rebased onto cbc380eb887ce65ddc7e33a180f90a5b41ebd385

3 years ago

Alright, removed the deprecation docstring.

rebased onto d8720ff

3 years ago

Whoops, forgot about that. Fixed.

Pull-Request has been merged by praiskup

3 years ago
Metadata
Flags
jenkins
failure
Build #143 failed (commit: d8720ff8)
3 years ago
Copr build
success (100%)
#1378950
3 years ago
Copr build
success (100%)
#1378949
3 years ago
Copr build
failure
#1378948
3 years ago
jenkins
failure
Build #142 failed (commit: cbc380eb)
3 years ago
Copr build
success (100%)
#1378932
3 years ago
Copr build
success (100%)
#1378931
3 years ago
Copr build
failure
#1378930
3 years ago
jenkins
failure
Build #138 failed (commit: 99f437e3)
3 years ago
Copr build
success (100%)
#1378544
3 years ago
Copr build
pending (50%)
#1378543
3 years ago
Copr build
failure
#1378542
3 years ago
jenkins
failure
Build #137 failed (commit: f0d39edc)
3 years ago
Copr build
success (100%)
#1378431
3 years ago
Copr build
success (100%)
#1378430
3 years ago
Copr build
success (100%)
#1378429
3 years ago
jenkins
failure
Build #136 failed (commit: 6db645c3)
3 years ago
Copr build
success (100%)
#1378278
3 years ago
Copr build
success (100%)
#1378276
3 years ago
Copr build
success (100%)
#1378274
3 years ago
jenkins
failure
Build #114 failed (commit: 096cb3d4)
3 years ago
Copr build
success (100%)
#1369227
3 years ago
Copr build
success (100%)
#1369226
3 years ago
Copr build
success (100%)
#1369225
3 years ago
jenkins
failure
Build #113 failed (commit: 7f67f314)
3 years ago
Copr build
success (100%)
#1369221
3 years ago
Copr build
success (100%)
#1369220
3 years ago
Copr build
success (100%)
#1369219
3 years ago
jenkins
failure
Build #109 failed (commit: ee8fa25f)
3 years ago
Copr build
success (100%)
#1368279
3 years ago
Copr build
success (100%)
#1368278
3 years ago
Copr build
success (100%)
#1368277
3 years ago
jenkins
failure
Build #107 failed (commit: fa39d10a)
3 years ago
Copr build
success (100%)
#1367950
3 years ago
Copr build
success (100%)
#1367949
3 years ago
Copr build
success (100%)
#1367948
3 years ago
jenkins
success (100%)
Build #43 successful (commit: 2be31e7d)
3 years ago
Copr build
success (100%)
#1342282
3 years ago
Copr build
success (100%)
#1342281
3 years ago
Copr build
failure
#1342280
3 years ago
Copr build
success (100%)
#1330094
3 years ago
Copr build
success (100%)
#1330093
3 years ago
Copr build
success (100%)
#1330092
3 years ago
Copr build
success (100%)
#1326927
3 years ago
Copr build
success (100%)
#1326926
3 years ago
Copr build
success (100%)
#1326925
3 years ago
Copr build
success (100%)
#1326260
3 years ago
Copr build
success (100%)
#1326259
3 years ago
Copr build
success (100%)
#1326258
3 years ago
Copr build
success (100%)
#1324913
3 years ago
Copr build
success (100%)
#1324912
3 years ago
Copr build
success (100%)
#1324911
3 years ago
Copr build
failure
#1323744
3 years ago
Copr build
failure
#1323743
3 years ago
Copr build
success (100%)
#1323742
3 years ago
Copr build
failure
#1323609
3 years ago
Copr build
failure
#1323608
3 years ago
Copr build
success (100%)
#1323607
3 years ago