copr / copr

Clone

#2072 cli: add --modules --with-opts and --without-opts to edit-chroot
Merged 2 years ago by praiskup. Opened 2 years ago by pbrezina.
copr/ pbrezina/copr climodules  into  main

file modified
+12 -5 
@@ -651,7 +651,6 @@ 
 

  

          :param args: argparse arguments provided by the user
 

          """
 

- 

          if args.bootstrap_image:
 

              args.bootstrap = 'image'
 

          owner, copr, chroot = self.parse_chroot_path(args.coprchroot)
 
@@ -659,7 +658,9 @@ 
 

              ownername=owner, projectname=copr, chrootname=chroot,
 

              comps=args.upload_comps, delete_comps=args.delete_comps,
 

              additional_packages=args.packages, additional_repos=args.repos,
 

-             bootstrap=args.bootstrap, bootstrap_image=args.bootstrap_image,
 

+             additional_modules=args.modules, with_opts=' '.join(args.rpmbuild_with),
 

+             without_opts=' '.join(args.rpmbuild_without), bootstrap=args.bootstrap,
 

+             bootstrap_image=args.bootstrap_image,
 

              isolation=args.isolation,
 

          )
 

          print("Edit chroot operation was successful.")
 
@@ -674,9 +675,9 @@ 
 

          project_chroot = self.client.project_chroot_proxy.get(
 

              ownername=owner, projectname=copr, chrootname=chroot
 

          )
 

-         fields = ["additional_packages", "additional_repos", "comps_name", "delete_after_days",
 

-                   "isolation", "mock_chroot", "ownername", "projectname", "with_opts",
 

-                   "without_opts"]
 

+         fields = ["additional_packages", "additional_repos", "additional_modules",
 

+                   "comps_name", "delete_after_days", "isolation", "mock_chroot",
 

+                   "ownername", "projectname", "with_opts", "without_opts"]
 

          printer = get_printer(args.output_format, fields)
 

          printer.add_data(project_chroot)
 

          printer.finish()
 
@@ -1404,6 +1405,12 @@ 
 

                                        help="space separated string of package names to be added to buildroot")
 

      parser_edit_chroot.add_argument("--repos",
 

                                        help="space separated string of additional repo urls for chroot")
 

+     parser_edit_chroot.add_argument("--modules",
 

+                                       help="comma separated list of modules that will be enabled or disabled in the given chroot, e.g. 'module1:stream,!module2:stream'")
 

+     parser_edit_chroot.add_argument("--rpmbuild-with", action='append', default=[],
 

+                                       help="rpmbuild --with option, can be set multiple times")
 

+     parser_edit_chroot.add_argument("--rpmbuild-without", action='append', default=[],
 

+                                       help="rpmbuild --without options, can be set multiple times")
 

      parser_edit_chroot.add_argument("--isolation", choices=["simple", "nspawn", "default"], default="unchanged",
 

                                      help="Choose the isolation method for running commands in buildroot.")

file modified
+7 -2 
@@ -17,6 +17,7 @@ 
 

          "comps_name": project_chroot.comps_name,
 

          "additional_repos": project_chroot.repos_list,
 

          "additional_packages": project_chroot.buildroot_pkgs_list,
 

+         "additional_modules": project_chroot.module_toggle,
 

          "with_opts": str_to_list(project_chroot.with_opts),
 

          "without_opts": str_to_list(project_chroot.without_opts),
 

          "delete_after_days": project_chroot.delete_after_days,
 
@@ -31,6 +32,7 @@ 
 

          "repos": config["repos"],
 

          "additional_repos": BuildConfigLogic.generate_additional_repos(project_chroot),
 

          "additional_packages": (project_chroot.buildroot_pkgs or "").split(),
 

+         "additional_modules": project_chroot.module_toggle,
 

          "enable_net": project_chroot.copr.enable_net,
 

          "with_opts":  str_to_list(project_chroot.with_opts),
 

          "without_opts": str_to_list(project_chroot.without_opts),
 
@@ -46,6 +48,7 @@ 
 

      replace = {
 

          "additional_repos": "repos",
 

          "additional_packages": "buildroot_pkgs",
 

+         "additional_modules": "module_toggle",
 

      }
 

      output = input.copy()
 

      for from_name, to_name in replace.items():
 
@@ -90,11 +93,13 @@ 
 

      if not form.validate_on_submit():
 

          raise InvalidForm(form)
 

  

-     buildroot_pkgs = repos = comps_xml = comps_name = with_opts = without_opts = None
 

+     buildroot_pkgs = repos = module_toggle = comps_xml = comps_name = with_opts = without_opts = None
 

      if "buildroot_pkgs" in data:
 

          buildroot_pkgs = form.buildroot_pkgs.data
 

      if "repos" in data:
 

          repos = form.repos.data
 

+     if "module_toggle" in data:
 

+         module_toggle = form.module_toggle.data
 

      if "with_opts" in data:
 

          with_opts = form.with_opts.data
 

      if "without_opts" in data:
 
@@ -106,7 +111,7 @@ 
 

          CoprChrootsLogic.remove_comps(flask.g.user, chroot)
 

      CoprChrootsLogic.update_chroot(
 

          flask.g.user, chroot, buildroot_pkgs, repos, comps=comps_xml, comps_name=comps_name,
 

-         with_opts=with_opts, without_opts=without_opts,
 

+         with_opts=with_opts, without_opts=without_opts, module_toggle=module_toggle,
 

          bootstrap=form.bootstrap.data,
 

          bootstrap_image=form.bootstrap_image.data,
 

          isolation=form.isolation.data)

file added
empty or binary file added
file modified
+3 -1 
@@ -49,7 +49,7 @@ 
 

  

      # pylint: disable=too-many-arguments
 

      def edit(self, ownername, projectname, chrootname, additional_packages=None, additional_repos=None,
 

-              comps=None, delete_comps=False, with_opts=None, without_opts=None,
 

+              additional_modules=None, comps=None, delete_comps=False, with_opts=None, without_opts=None,
 

               bootstrap=None, bootstrap_image=None, isolation=None):
 

          """
 

          Edit a chroot configuration in a project
 
@@ -59,6 +59,7 @@ 
 

          :param str chrootname:
 

          :param list additional_packages: buildroot packages for the chroot
 

          :param list additional_repos: buildroot additional additional_repos
 

+         :param list additional_modules: additional modules for the chroot
 

          :param str comps: file path to the comps.xml file
 

          :param bool delete_comps: if True, current comps.xml will be removed
 

          :param list with_opts: Mock --with option
 
@@ -83,6 +84,7 @@ 
 

          data = {
 

              "additional_repos": additional_repos,
 

              "additional_packages": additional_packages,
 

+             "additional_modules": additional_modules,
 

              "delete_comps": delete_comps,
 

              "with_opts": with_opts,
 

              "without_opts": without_opts,

It probably requires some tests, but I need some help with it. I'm not really sure which test should I modify and how.

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

rebased onto 454747c30afcdb5c97b7559d656094bdfff6ea93
2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

I'm also not sure what to do with this:

copr/v3/proxies/project_chroot.py:51:4: R0914[too-many-locals]: ProjectChrootProxy.edit: Too many local variables (21/20)

copr/v3/proxies/project_chroot.py:51:4: R0914[too-many-locals]: ProjectChrootProxy.edit: Too many local variables (21/20)

Ignore this one.

Can you please add a longer description, including two examples (enabling and disabling module)?

Can you change this to --rpmbuild-with <value> --rpmbuild-with <value 2> ...? Also the description should claim that it is used like rpmbuild --with <value>.

It probably requires some tests, but I need some help with it.

There's one test-case broken by this PR, you could use that one as an example.
Perhaps that, when fixed, will be good enough? I would take a look at manual
testing of this feature next week.

And thank you very much for your contribution!

Can you change this to --rpmbuild-with <value> --rpmbuild-with <value 2> ...? Also the description should claim that it is used like rpmbuild --with <value>.

I took the description from webui (text field placeholder), perhaps it should be changed there as well?

It probably requires some tests, but I need some help with it.

There's one test-case broken by this PR, you could use that one as an example.
Perhaps that, when fixed, will be good enough? I would take a look at manual
testing of this feature next week.

How can I run the tests locally?

I took the description from webui (text field placeholder), perhaps it should be changed there as well?

In web-UI is something like:

You can specify rpmbuild --without options here for builds in the given chroot.
Space separated list of the rpmbuild without options

But enhancements there are OK as well :-)

How can I run the tests locally?

$ cd frontend && ./run_tests.sh

rebased onto 09f9c052751d6f72091482f35af4387ce2c0d811
2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

rebased onto 1c5b5b36f02a2134234d74a89afab04dadf18beb
2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

enabled or disabled

This raises TypeError: can only join an iterable when not specified. So I suppose the default=[] needs to be specified on the parser side.

--rpmbuild-with AAA --rpmbuild-with BBB sets just BBB

But I'm now more worried about how to properly send a "reset" action. This has never
been done before? @frostyx, ideas?

When edit-chroot is specified without those options, nothing changes. When
--rpmbuild-with OPT is specified, everything is re-set and OPT is in effect.
Now, when --rpmbuild-with OPT2 is specified, OPT is re-set.

rebased onto 5c3794ff969d6f6191edfe84c28826d3f8137e7f
2 years ago

enabled or disabled

Fixed

This raises TypeError: can only join an iterable when not specified. So I suppose the default=[] needs to be specified on the parser side.

Fixed

--rpmbuild-with AAA --rpmbuild-with BBB sets just BBB

Fixed, switched to action="append".
Edited 2 years ago by pbrezina

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

With the current code, the following command just re-sets the "with options" field to an empty string:

$ copr edit-chroot praiskup/ping/fedora-rawhide-x86_64

It should be a no-op from the --rpmbuild-with/--rpmbuild-without perspective. Meh.

So, I think it is OK as it is, with the only exception: Omitting any of those three new options shouldn't reset the config.

Then there's a question how to actually clear the config when user wants to. Perhaps
we could have --no-rpmbuild-with?

This should be discussed, @frostyx, wdyt?

Of course, @schlupov too ^^, @frostyx is our notorious API contributor though.

Metadata Update from @praiskup:
- Pull-request tagged with: review
2 years ago

I am not sure if it is a good idea to go with additional_modules naming because the field is used for both enabling but also disabling modules from the buildroot.

Then there's a question how to actually clear the config when user wants to. Perhaps
we could have --no-rpmbuild-with?

Possibly. Or we could add --reset <name>, --default <name>, --reset-to-default <name>, or something like this, that would take the field name. The advantage is that it could be easily used for any other option.

But I am a bit lost how is this a new situation different from editing project settings. I think we could inspire from apiv3_projects.py:edit_project function. In it, we distinguish between

  • None - Ignore the field and leave it unchanged
  • empty string - Reset the field value

So for resetting the value via CLI, one could do copr-cli ... --modules "".

 
None - Ignore the field and leave it unchanged

This should be (I believe it already is/was) applied here in this PR.

empty string - Reset the field value

This is a bit fuzzy IMO. Considering empty value can have its own semantics
than re-set for some fields (implemented in the future).

I kind of like the --reset idea, but mapping the argument to appropriate
field or command line option is not easy I guess (--reset rpmbuild-with ?).

I kind of like the --reset idea, but mapping the argument to appropriate
field or command line option is not easy I guess (--reset rpmbuild-with ?).

I think we should match the field names that get-chroot (or other) command returns, e.g.

$ copr-cli get-chroot @copr/copr/fedora-rawhide-x86_64
{
    "additional_packages": [],
    "additional_repos": [],
    "comps_name": null,
    "delete_after_days": null,
    "isolation": null,
    "mock_chroot": "fedora-rawhide-x86_64",
    "ownername": "@copr",
    "projectname": "copr",
    "with_opts": [],
    "without_opts": []
}

So e.g. --reset with_opts. It makes the most sense to me but we don't have to do it this way.

@pbrezina, thanks for your contribution. We agreed with @frostyx that we'll continue on top of this PR ... merging!

Great, thank you. Is there any ETA when this will be available in pypi?

Pull-Request has been merged by praiskup
2 years ago

Jakub is going to write the PR first, and then the release -- and it's not yet on the schedule. We do release cca each 2 to 3 months, last 14 old..
https://docs.pagure.org/copr.copr/release-notes/2021-11-11.html
Metadata
None

review

Changes Summary 4
+12 -5
file changed
cli/copr_cli/main.py
+7 -2
file changed
frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_project_chroots.py
+0
file added
frontend/dump.rdb
+3 -1
file changed
python/copr/v3/proxies/project_chroot.py
Powered by Pagure 5.14.1
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.