#2072 cli: add --modules --with-opts and --without-opts to edit-chroot
Merged 5 months ago by praiskup. Opened 5 months 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.")

  

@@ -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)

empty or binary file added
@@ -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

5 months 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

5 months ago

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

rebased onto 1c5b5b36f02a2134234d74a89afab04dadf18beb

5 months ago

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

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

5 months 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".

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

5 months 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

5 months 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