PR#2117: Implement edit-chroot --reset option - copr/copr

		`@@ -97,6 +97,8 @@`
		`case $_frontend_url in`
		`*copr.stg.fedoraproject.org) ;;`
		`*dev-copr.devel.redhat.com) ;;`
		`+ # https://docs.pagure.org/copr.copr/sanity_tests.html#sanity-tests`
		`+ *frontend ;;`
		`*) rlDie "improper copr_url in $_config" ;;`
		`esac`

beaker-tests/Sanity/copr-cli-basic-operations/runtest-reset-to-defaults.sh

file added

+95

		`@@ -0,0 +1,95 @@`
		`+ #! /bin/bash`
		`+ #`
		`+ # Copyright (c) 2022 Red Hat, Inc.`
		`+ #`
		`+ # This program is free software: you can redistribute it and/or`
		`+ # modify it under the terms of the GNU General Public License as`
		`+ # published by the Free Software Foundation, either version 2 of`
		`+ # the License, or (at your option) any later version.`
		`+ #`
		`+ # This program is distributed in the hope that it will be`
		`+ # useful, but WITHOUT ANY WARRANTY; without even the implied`
		`+ # warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR`
		`+ # PURPOSE. See the GNU General Public License for more details.`
		`+ #`
		`+ # You should have received a copy of the GNU General Public License`
		`+ # along with this program. If not, see http://www.gnu.org/licenses/.`
		`+`
		`+`
		`+ # Include Beaker environment`
		`+ . /usr/share/beakerlib/beakerlib.sh \|\| exit 1`
		`+`
		`+ # Load config settings`
		`+ HERE=$(dirname "$(realpath "$0")")`
		`+ source "$HERE/config"`
		`+ source "$HERE/helpers"`
		`+`
		`+ modify_chroot()`
		`+ {`
		`+ copr-cli edit-chroot $PROJECT/$CHROOT \`
		`+ --isolation simple \`
		`+ --modules "foo:bar" \`
		`+ --packages "foo bar baz" \`
		`+ --rpmbuild-with="--foo"`
		`+ }`
		`+`
		`+ rlJournalStart`
		`+ rlPhaseStartSetup`
		`+ setup_checks`
		`+ rlAssertRpm "jq"`
		`+ workdirSetup`
		`+ setupProjectName "ResetToDefaults"`
		`+ rlPhaseEnd`
		`+`
		`+ rlPhaseStartTest`
		`+ # Create an empty project and save the chroot defaults`
		`+ tmp1=$(mktemp -p ./)`
		`+ rlRun "copr-cli create --chroot $CHROOT $PROJECT"`
		`+ rlRun "copr-cli get-chroot $PROJECT/$CHROOT > $tmp1"`
		`+`
		`+ # Modify the chroot and configure various options`
		`+ tmp=$(mktemp -p ./)`
		`+ rlRun "modify_chroot"`
		`+ rlRun "copr-cli get-chroot $PROJECT/$CHROOT > $tmp"`
		`+ rlRun "jq -e '.additional_packages == [\"foo\", \"bar\", \"baz\"]' $tmp"`
		`+ rlRun "jq -e '.additional_modules == [\"foo:bar\"]' $tmp"`
		`+ rlRun "jq -e '.isolation == \"simple\"' $tmp"`
		`+ rlRun "jq -e '.with_opts == [\"--foo\"]' $tmp"`
		`+`
		`+ # Reset one field and make sure the rest of them remain unchanged`
		`+ rlRun "copr-cli edit-chroot $PROJECT/$CHROOT --reset additional_packages"`
		`+ rlRun "copr-cli get-chroot $PROJECT/$CHROOT > $tmp"`
		`+ rlRun "jq -e '.additional_packages == []' $tmp"`
		`+ rlRun "jq -e '.additional_modules == [\"foo:bar\"]' $tmp"`
		`+ rlRun "jq -e '.isolation == \"simple\"' $tmp"`
		`+ rlRun "jq -e '.with_opts == [\"--foo\"]' $tmp"`
		`+`
		`+ # Reset another field`
		`+ rlRun "copr-cli edit-chroot $PROJECT/$CHROOT --reset additional_modules"`
		`+ rlRun "copr-cli get-chroot $PROJECT/$CHROOT > $tmp"`
		`+ rlRun "jq -e '.additional_modules == []' $tmp"`
		`+ rlRun "jq -e '.isolation == \"simple\"' $tmp"`
		`+ rlRun "jq -e '.with_opts == [\"--foo\"]' $tmp"`
		`+`
		`+ # Reset multiple fields at once`
		`+ rlRun "copr-cli edit-chroot $PROJECT/$CHROOT --reset isolation --reset with_opts"`
		`+ rlRun "copr-cli get-chroot $PROJECT/$CHROOT > $tmp"`
		`+ rlRun "jq -e '.isolation == \"unchanged\"' $tmp"`
		`+ rlRun "jq -e '.with_opts == []' $tmp"`
		`+`
		`+ # When all fields have been reseted, make sure the chroot is exactly`
		`+ # same as when it was fresh`
		`+ rlRun "diff $tmp1 $tmp"`
		`+`
		`+ # Try to reset a non-existing field`
		`+ rlRun "copr-cli edit-chroot $PROJECT/$CHROOT --reset nonexisting &> $tmp" 1`
		`+ rlRun "cat $tmp \|grep 'Trying to reset an invalid attribute: nonexisting'"`
		`+ rlRun "cat $tmp \|grep \"copr-cli get-chroot $PROJECT/$CHROOT' for all the possible attributes\""`
		`+ rlPhaseEnd`
		`+`
		`+ rlPhaseStartCleanup`
		`+ cleanProject`
		`+ workdirCleanup`
		`+ rlPhaseEnd`
		`+ rlJournalPrintText`
		`+ rlJournalEnd`

cli/copr_cli/main.py

file modified

+24 -5

		`@@ -654,14 +654,24 @@`
		`if args.bootstrap_image:`
		`args.bootstrap = 'image'`
		`owner, copr, chroot = self.parse_chroot_path(args.coprchroot)`
		`+`
		`+ with_opts = None`
		`+ if args.rpmbuild_with:`
		`+ with_opts = ' '.join(args.rpmbuild_with)`
		`+`
		`+ without_opts = None`
		`+ if args.rpmbuild_without:`
		`+ without_opts = ' '.join(args.rpmbuild_without)`
		`+`
		`self.client.project_chroot_proxy.edit(`
		`ownername=owner, projectname=copr, chrootname=chroot,`
		`comps=args.upload_comps, delete_comps=args.delete_comps,`
		`additional_packages=args.packages, additional_repos=args.repos,`
		`- additional_modules=args.modules, with_opts=' '.join(args.rpmbuild_with),`
		`- without_opts=' '.join(args.rpmbuild_without), bootstrap=args.bootstrap,`
		`+ additional_modules=args.modules, with_opts=with_opts,`
		`+ without_opts=without_opts, bootstrap=args.bootstrap,`
		`bootstrap_image=args.bootstrap_image,`
		`isolation=args.isolation,`
		`+ reset_fields=args.reset,`
		`)`
		`print("Edit chroot operation was successful.")`

		`@@ -1407,11 +1417,11 @@`
		`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=[],`
		`+ parser_edit_chroot.add_argument("--rpmbuild-with", action='append',`
		`help="rpmbuild --with option, can be set multiple times")`
		`- parser_edit_chroot.add_argument("--rpmbuild-without", action='append', default=[],`
		`+ parser_edit_chroot.add_argument("--rpmbuild-without", action='append',`
		`help="rpmbuild --without options, can be set multiple times")`
		`- parser_edit_chroot.add_argument("--isolation", choices=["simple", "nspawn", "default"], default="unchanged",`
		`+ parser_edit_chroot.add_argument("--isolation", choices=["simple", "nspawn", "default"],`
		`help="Choose the isolation method for running commands in buildroot.")`

		`parser_edit_chroot.add_argument(`
		`@@ -1429,6 +1439,15 @@`
		`help=("Use a custom container image for initializing Mock's "`
		`"bootstrap (Implies --bootstrap=image)"))`

		`+ parser_edit_chroot.add_argument(`
		`+ "--reset",`
		`+ action="append",`
		`+ help=("Reset this parameters to their respective defaults. "`
		`+ "Possible values are additional_packages, additional_modules, "`
		+ "isolation, etc. See the output of `copr-cli get-chroot' for all "
		`+ "the possible field names."),`
		`+ )`
		`+`
		`parser_edit_chroot.set_defaults(func="action_edit_chroot")`

		`parser_get_chroot = subparsers.add_parser("get-chroot", help="Get chroot of a project")`

frontend/coprs_frontend/coprs/forms.py

file modified

		`@@ -1589,6 +1589,7 @@`
		`comps = None`
		`upload_comps = FileField("Upload comps.xml")`
		`delete_comps = wtforms.BooleanField("Delete comps.xml", false_values=FALSE_VALUES)`
		`+ reset_fields = wtforms.StringField("Reset these fields to their defaults")`


		`class SelectMultipleFieldNoValidation(wtforms.SelectMultipleField):`

frontend/coprs_frontend/coprs/models.py

file modified

		`@@ -1677,6 +1677,14 @@`
		`return self.mock_chroot.name`

		`@property`
		`+ def full_name(self):`
		`+ """`
		`+ Return a full path for identifying some chroot, e.g.`
		+ `@copr/copr-dev/fedora-rawhide-x86_64`
		`+ """`
		`+ return "{0}/{1}".format(self.copr.full_name, self.name)`
		`+`
		`+ @property`
		`def is_active(self):`
		`return self.mock_chroot.is_active`

frontend/coprs_frontend/coprs/views/apiv3_ns/__init__.py

file modified

+45

		`@@ -14,6 +14,7 @@`
		`CoprHttpException,`
		`InsufficientStorage,`
		`ObjectNotFound,`
		`+ BadRequest,`
		`)`
		`from coprs.logic.complex_logic import ComplexLogic`

		`@@ -291,3 +292,47 @@`
		`_stream(),`
		`mimetype="application/json",`
		`)`
		`+`
		`+`
		`+ def str_to_list(value):`
		`+ """`
		`+ We have a lot of module attributes that are stored as space-separeted`
		+ strings, their default value is `None` and we want to return them as lists.
		+ It's tiresome to always check if the value is not `None` before splitting.
		`+ """`
		`+ return (value or "").split()`
		`+`
		`+`
		`+ def reset_to_defaults(obj, form, rename_fields=None, more_fields=None):`
		`+ """`
		+ If the `form` contains a `reset` with a list of attributes that user wishes
		`+ to reset, set such values to their respective defaults. We find out the`
		+ default value from `obj`, which should be any database model.
		`+`
		`+ Sometimes an attribute within the API schema can have a different name than`
		+ its model counterpart. For such cases this method takes an `rename_fields`,
		+ function, see e.g. `apiv3_project_chroots.rename_fields`.
		`+`
		+ Use `more_fields` to specify a message saying where a user can find all the
		`+ possible attribute names`
		`+ """`
		`+ reset = getattr(form, "reset_fields")`
		`+ if not reset or not reset.data:`
		`+ return`
		`+ fields = str_to_list(reset.data)`
		`+`
		`+ # Some fields may have different name in the API schema than their`
		`+ # respective attributes in the database models.`
		`+ if rename_fields:`
		`+ fields = rename_fields(dict.fromkeys(fields)).keys()`
		`+`
		`+ for field in fields:`
		`+ try:`
		`+ default = getattr(obj.__class__, field).default`
		`+ value = default.arg if default else None`
		`+ setattr(obj, field, value)`
		`+ except AttributeError as ex:`
		`+ msg = "Trying to reset an invalid attribute: {0}".format(field)`
		`+ if more_fields:`
		`+ msg += "\n{0}".format(more_fields)`
		`+ raise BadRequest(msg) from ex`

frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_project_chroots.py

file modified

+15 -7

		`@@ -5,7 +5,15 @@`
		`from coprs.exceptions import ObjectNotFound, InvalidForm`
		`from coprs import db, forms`
		`from coprs.logic.coprs_logic import CoprChrootsLogic`
		`- from . import query_params, get_copr, file_upload, GET, PUT`
		`+ from . import (`
		`+ query_params,`
		`+ get_copr,`
		`+ file_upload,`
		`+ GET,`
		`+ PUT,`
		`+ str_to_list,`
		`+ reset_to_defaults,`
		`+ )`
		`from .json2form import get_form_compatible_data`


		`@@ -17,7 +25,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,`
		`+ "additional_modules": str_to_list(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,`
		`@@ -32,7 +40,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,`
		`+ "additional_modules": str_to_list(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),`
		`@@ -58,10 +66,6 @@`
		`return output`


		`- def str_to_list(value):`
		`- return (value or "").split()`
		`-`
		`-`
		`@apiv3_ns.route("/project-chroot", methods=GET)`
		`@query_params()`
		`def get_project_chroot(ownername, projectname, chrootname):`
		`@@ -93,6 +97,10 @@`
		`if not form.validate_on_submit():`
		`raise InvalidForm(form)`

		+ more_fields = ("See `copr-cli get-chroot {0}' for all the possible "
		`+ "attributes".format(chroot.full_name))`
		`+ reset_to_defaults(chroot, form, rename_fields, more_fields)`
		`+`
		`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`

frontend/coprs_frontend/tests/request_test_api.py

file modified

+10 -3

		`@@ -50,7 +50,8 @@`
		`raise NotImplementedError`

		`def edit_chroot(self, project, chroot, bootstrap=None,`
		`- bootstrap_image=None, owner=None, isolation=None):`
		`+ bootstrap_image=None, owner=None, isolation=None,`
		`+ additional_packages=None, reset_fields=None):`
		`""" Modify CoprChroot """`
		`raise NotImplementedError`

		`@@ -94,7 +95,8 @@`
		`return resp`

		`def edit_chroot(self, project, chroot, bootstrap=None,`
		`- bootstrap_image=None, owner=None, isolation=None):`
		`+ bootstrap_image=None, owner=None, isolation=None,`
		`+ additional_packages=None, reset_fields=None):`
		`""" Change CoprChroot using the web-UI """`
		`route = "/coprs/{user}/{project}/update_chroot/{chroot}/".format(`
		`user=owner or self.transaction_username,`
		`@@ -233,7 +235,8 @@`
		`return resp`

		`def edit_chroot(self, project, chroot, bootstrap=None,`
		`- bootstrap_image=None, owner=None, isolation=None):`
		`+ bootstrap_image=None, owner=None, isolation=None,`
		`+ additional_packages=None, reset_fields=None):`
		`route = "/api_3/project-chroot/edit/{owner}/{project}/{chroot}".format(`
		`owner=owner or self.transaction_username,`
		`project=project,`
		`@@ -246,6 +249,10 @@`
		`data["bootstrap_image"] = bootstrap_image`
		`if isolation is not None:`
		`data["isolation"] = isolation`
		`+ if additional_packages is not None:`
		`+ data["additional_packages"] = additional_packages`
		`+ if reset_fields is not None:`
		`+ data["reset_fields"] = reset_fields`
		`resp = self.post(route, data)`
		`return resp`

frontend/coprs_frontend/tests/test_apiv3/test_copr_chroot.py

file modified

+54

		`@@ -55,6 +55,60 @@`
		`for chroot in copr.active_copr_chroots:`
		`assert chroot.isolation == "nspawn"`

		`+ @TransactionDecorator("u1")`
		`+ @pytest.mark.usefixtures("f_users", "f_users_api", "f_mock_chroots", "f_db")`
		`+ def test_v3_edit_chroot_reset(self):`
		`+ chrootname = "fedora-rawhide-i386"`
		`+ project = "test"`
		`+`
		`+ # Create a new project and edit some chroot attributes`
		`+ self.api3.new_project(project, [chrootname])`
		`+ self.api3.edit_chroot(`
		`+ project,`
		`+ chrootname,`
		`+ additional_packages=["pkg1", "pkg2", "pkg3"],`
		`+ isolation="nspawn",`
		`+ )`
		`+`
		`+ # Make sure all the chroot attributes are configured`
		`+ chroot = self.models.CoprChroot.query.get(1)`
		`+ assert chroot.isolation == "nspawn"`
		`+ assert chroot.buildroot_pkgs == "pkg1 pkg2 pkg3"`
		`+`
		`+ # Reset one of the fields and make sure nothing else was changed`
		`+ response = self.api3.edit_chroot(`
		`+ project,`
		`+ chrootname,`
		`+ reset_fields=["additional_packages"]`
		`+ )`
		`+ assert response.status_code == 200`
		`+ chroot = self.models.CoprChroot.query.get(1)`
		`+ assert chroot.isolation == "nspawn"`
		`+ assert chroot.buildroot_pkgs is None`
		`+`
		`+ # Reset the rest of the fields`
		`+ response = self.api3.edit_chroot(`
		`+ project,`
		`+ chrootname,`
		`+ reset_fields=["additional_packages", "isolation"]`
		`+ )`
		`+ chroot = self.models.CoprChroot.query.get(1)`
		`+ assert chroot.isolation == "unchanged"`
		`+ assert chroot.buildroot_pkgs is None`
		`+`
		`+ # Try to reset a non-existing attribute`
		`+ response = self.api3.edit_chroot(`
		`+ project,`
		`+ chrootname,`
		`+ reset_fields=["nonexisting"],`
		`+ )`
		`+ assert response.status_code == 400`
		`+ assert ("Trying to reset an invalid attribute: nonexisting"`
		`+ in response.json["error"])`
		+ assert ("See `copr-cli get-chroot user1/test/fedora-rawhide-i386' for "
		`+ "all the possible attributes"`
		`+ in response.json["error"])`
		`+`
		`@TransactionDecorator("u2")`
		`@pytest.mark.usefixtures("f_users", "f_users_api", "f_mock_chroots", "f_db")`
		`def test_edit_chroot_permission(self):`

frontend/dump.rdb

file removed

python/copr/v3/proxies/project_chroot.py

file modified

+7 -1

		`@@ -50,7 +50,8 @@`
		`# pylint: disable=too-many-arguments`
		`def edit(self, ownername, projectname, chrootname, additional_packages=None, additional_repos=None,`
		`additional_modules=None, comps=None, delete_comps=False, with_opts=None, without_opts=None,`
		`- bootstrap=None, bootstrap_image=None, isolation=None):`
		`+ bootstrap=None, bootstrap_image=None, isolation=None,`
		`+ reset_fields=None):`
		`"""`
		`Edit a chroot configuration in a project`

		`@@ -69,6 +70,10 @@`
		`:param str bootstrap_image: Implies 'bootstrap=image'.`
		`:param str isolation: Mock isolation feature setup.`
		`Possible values are 'default', 'simple', 'nspawn'.`
		`+ :param list reset_fields: list of chroot attributes, that should be`
		`+ reseted to their respective defaults. Possible values are`
		+ `additional_packages`, `additional_modules`, `isolation`, etc. See
		+ the output of `ProjectProxy.get` for all the possible field names.
		`:return: Munch`
		`"""`
		`endpoint = "/project-chroot/edit/{ownername}/{projectname}/{chrootname}"`
		`@@ -91,6 +96,7 @@`
		`"bootstrap": bootstrap,`
		`"bootstrap_image": bootstrap_image,`
		`"isolation": isolation,`
		`+ "reset_fields": reset_fields,`
		`}`
		`files = {}`
		`if comps:`

frostyx commented 2 years ago

See PR#2072

There are multiple commits that are not that related to each other. Please see their descriptions.

Metadata Update from @frostyx:
- Pull-request tagged with: wip

2 years ago

4 new commits added

frontend, python, cli: add support for resetting fields
cli: change edit-chroot defaults to None
frontend: return chroot modules as a list
beaker-tests-sanity: allow running against docker instance

2 years ago

frostyx commented 2 years ago

It works but I wasn't sure how to implement this, so please see if you like the general idea here.
I will polish it afterward.

zuul commented 2 years ago

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

csdiff-pylint : FAILURE in 1m 59s

praiskup commented on line 50 of cli/copr_cli/main.py 2 years ago

What I don't like is the absence of possible fields to be reset ... can we print this somehow?

Metadata Update from @frostyx:
- Pull-request tagged with: release-blocker

2 years ago

frostyx commented on line 23 of frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_project_chroots.py 2 years ago

I am adding a release-blocker tag for this PR because of this change.

praiskup commented on line 14 of python/copr/v3/proxies/project_chroot.py 2 years ago

this needs a docstring update

praiskup commented 2 years ago

Hm.. will you have time to rerun beaker tests?

4 new commits added

frontend, python, cli: add support for resetting fields
cli: change edit-chroot defaults to None
frontend: return chroot modules as a list
beaker-tests-sanity: allow running against docker instance

2 years ago

rebased onto dd53bcb3059da7f6b0c66560ac40244652b33b64

2 years ago

zuul commented 2 years ago

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

csdiff-pylint : FAILURE in 2m 22s

4 new commits added

frontend, python, cli: add support for resetting fields
cli: change edit-chroot defaults to None
frontend: return chroot modules as a list
beaker-tests-sanity: allow running against docker instance

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 1m 55s

Metadata Update from @frostyx:
- Pull-request untagged with: wip

2 years ago

frostyx commented 2 years ago

Hm.. will you have time to rerun beaker tests?

Running right now

this needs a docstring update

Updated

What I don't like is the absence of possible fields to be reset ... can we print this somehow?

Updated

I have also added a test for resetting a non-existing attribute, changed the uppercase RESET to just normal reset, fixed zuul warning, and such.

The copr-cli fails to build for EPEL7 with E ImportError: No module named requests_gssapi which I had no idea where it comes from. But I would say it should be unrelated to this PR.

Also, I think the --reset parameter should be implemented also for copr-cli modify command but since this is a release blocker, I maybe we can do it later in a follow-up PR?

praiskup commented on line 6 of python/copr/v3/proxies/project_chroot.py 2 years ago

Could we name it reset_fields, so it has less chance to collide with others?

Edited 2 years ago by praiskup

praiskup commented on line 59 of frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_project_chroots.py 2 years ago

I know you claimed why this can not be done before update_chroot() but I can't recall the reason?

praiskup commented 2 years ago

Would you mind adding a simple test for the reset behavior into:
coprs_frontend/tests/test_apiv3/test_copr_chroot.py?
The self.web_ui.edit_chroot can be fixed to support "resetting"..