PR#435: Fix `copr-cli mock-config` after switching to APIv3 - copr/copr

		`@@ -1,5 +1,6 @@`
		`# coding: utf-8`

		`+ import re`
		`from jinja2 import Environment`

		`template_string = """\`
		`@@ -10,7 +11,7 @@`

		`include('/etc/mock/{{chroot}}.cfg')`

		`- config_opts['root'] = '{{project_id}}_{{chroot}}'`
		`+ config_opts['root'] = '{{ fullname \|project_id }}_{{chroot}}'`
		`config_opts['chroot_additional_packages'] = '`
		`{%- for pkg in additional_packages -%}`
		`{%- if loop.last -%}`
		`@@ -19,12 +20,12 @@`
		`{{ pkg }} {% endif -%}`
		`{%- endfor -%}'`

		`- {% if repos %}`
		`+ {% if additional_repos %}`
		`config_opts['yum.conf'] += \"\"\"`
		`- {% for repo in repos %}`
		`- [{{ repo.id }}]`
		`- name="{{ repo.name }}"`
		`- baseurl={{ repo.url }}`
		`+ {% for repo in additional_repos %}`
		`+ [{{ repo \|repo_name }}]`
		`+ name="Additional repo {{ repo \|repo_name }}"`
		`+ baseurl={{ repo }}`
		`gpgcheck=0`
		`enabled=1`
		`skip_if_unavailable=1`
		`@@ -41,8 +42,21 @@`
		`self.data = data`

		`def __str__(self):`
		`- template = Environment().from_string(template_string)`
		`+ env = Environment()`
		`+ env.filters["repo_name"] = generate_repo_name`
		`+ env.filters["project_id"] = project_id`
		`+ template = env.from_string(template_string)`
		`return template.render(self.data)`


		`+ def generate_repo_name(repo_url):`
		`+ """ based on url, generate repo name """`
		`+ repo_url = re.sub("[^a-zA-Z0-9]", '_', repo_url)`
		`+ repo_url = re.sub("(__*)", '_', repo_url)`
		`+ repo_url = re.sub("(_$)\|^_", '', repo_url)`
		`+ return repo_url`

		`+`
		`+ def project_id(fullname):`
		`+ ownername, name = fullname.split("/")`
		`+ return "{}-{}".format(ownername.replace("@", "group_"), name)`

cli/copr_cli/main.py

file modified

+3 -1

		`@@ -406,7 +406,9 @@`
		`"""`
		`sys.stderr.write("# This command is deprecated and will be removed in a future release.\n")`
		`ownername, projectname = self.parse_name(args.project)`
		`- build_config = self.client.project_chroot_proxy.get_build_config(ownername, projectname, args.chroot)`
		`+ build_config = self.client.project_chroot_proxy.get_build_config(ownername, projectname, args.chroot,`
		`+ expand_repourls=True)`
		`+ build_config["fullname"] = "/".join([ownername, projectname])`
		`print(MockProfile(build_config))`

frontend/coprs_frontend/coprs/helpers.py

file modified

+4 -1

		`@@ -547,11 +547,14 @@`
		`}`


		`- def generate_additional_repos(copr_chroot):`
		`+ def generate_additional_repos(copr_chroot, preprocess=False):`
		`base_repo = "copr://{}".format(copr_chroot.copr.full_name)`
		`repos = [base_repo] + copr_chroot.repos_list + copr_chroot.copr.repos_list`
		`if not copr_chroot.copr.auto_createrepo:`
		`repos.append("copr://{}/devel".format(copr_chroot.copr.full_name))`
		`+`
		`+ if preprocess:`
		`+ return [pre_process_repo_url(copr_chroot.name, r) for r in repos]`
		`return repos`

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

file modified

+8 -1

		`@@ -53,7 +53,14 @@`
		`# If parameter has a default value, it is not required`
		`if sig.parameters[arg].default == sig.parameters[arg].empty:`
		`raise CoprHttpException("Missing argument {}".format(arg))`
		`- kwargs[arg] = flask.request.args.get(arg)`
		`+`
		`+ # Convert "True"/"False" to boolean`
		`+ value = flask.request.args.get(arg)`
		`+ if type(sig.parameters[arg].default) == bool:`
		`+ value = value == "True"`
		`+`
		`+ kwargs[arg] = value`
		`+`
		`return f(args, *kwargs)`
		`return query_params_wrapper`
		`return query_params_decorator`

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

file modified

+4 -4

		`@@ -23,11 +23,11 @@`
		`}`


		`- def to_build_config_dict(project_chroot):`
		`+ def to_build_config_dict(project_chroot, expand_repourls=False):`
		`return {`
		`"chroot": project_chroot.name,`
		`"additional_packages": (project_chroot.buildroot_pkgs or "").split(),`
		`- "additional_repos": generate_additional_repos(project_chroot),`
		`+ "additional_repos": generate_additional_repos(project_chroot, preprocess=expand_repourls),`
		`"use_bootstrap_container": project_chroot.copr.use_bootstrap_container,`
		`"enable_net": project_chroot.copr.enable_net,`
		`"with_opts": str_to_list(project_chroot.with_opts),`
		`@@ -62,12 +62,12 @@`

		`@apiv3_ns.route("/project-chroot/build-config", methods=GET)`
		`@query_params()`
		`- def get_build_config(ownername, projectname, chrootname):`
		`+ def get_build_config(ownername, projectname, chrootname, expand_repourls=False):`
		`copr = get_copr(ownername, projectname)`
		`chroot = ComplexLogic.get_copr_chroot_safe(copr, chrootname)`
		`if not chroot:`
		`raise ObjectNotFound('Chroot not found.')`
		`- config = to_build_config_dict(chroot)`
		`+ config = to_build_config_dict(chroot, expand_repourls=expand_repourls)`
		`return flask.jsonify(config)`

python/copr/v3/proxies/project_chroot.py

file modified

+3 -1

		`@@ -26,13 +26,14 @@`
		`response = request.send()`
		`return munchify(response)`

		`- def get_build_config(self, ownername, projectname, chrootname):`
		`+ def get_build_config(self, ownername, projectname, chrootname, expand_repourls=False):`
		`"""`
		`Return a build configuration of a chroot in a project`

		`:param str ownername:`
		`:param str projectname:`
		`:param str chrootname:`
		`+ :param bool expand_repourls: Expand copr://<owner>/<projecct> to fully qualified URLs`
		`:return: Munch`
		`"""`
		`endpoint = "/project-chroot/build-config"`
		`@@ -40,6 +41,7 @@`
		`"ownername": ownername,`
		`"projectname": projectname,`
		`"chrootname": chrootname,`
		`+ "expand_repourls": expand_repourls,`
		`}`
		`request = Request(endpoint, api_base_url=self.api_base_url, params=params)`
		`response = request.send()`

frostyx commented 5 years ago

This PR is trying to fix #427.
The issue is in depth explained in 405#comment-65090.

Do you consider this a good enough solution?

but if we plan to release a new copr-cli package we should flip this back to api v1 call for the time being

Since the rest of the copr-cli code switched to using APIv3 client and this would be the only action using the APIv1, I would rather fix this straight away, than "temporarily" switching to the old client. It wouldn't be that straightforward and some additional code would be necessary to e.g. catch the exceptions.

add a separate call, named e.g. /api_3/backend-results/

As I unfortunately didn't attend the meeting, I don't know the context of this, but I don't think that another API call is needed.

add /project-chroot/build-config?expand_repourls=1

Yep, I like this the most and this PR implements it. The only exception is that it doesn't implement the expand_repourls=1 but returns preprocessed URLs straight away. I think, that it should be just fine, but if the parameter is preferred, I can easily add it.

msuchy commented 5 years ago

+1
and +1 to expand_repourls=1

frostyx commented 5 years ago

and +1 to expand_repourls=1

Yea, thinking about it again, we should add the parameter. Additional repos for build chroot are not preprocessed/expanded and we want to keep them that way, so having expanded repos for mock chroot by default would be inconsistent.

praiskup commented 5 years ago

I tried:

$ ./copr mock-config praiskup/ping fedora-rawhide-x86_64
...
baseurl=copr://praiskup/ping
...

Otherwise +1.

frostyx commented 5 years ago

I tried:

@praiskup, have you tried it against frontend from this PR?

2 new commits added

[frontend][python][cli] preprocess copr:// URLs based on expand_repourls parameter
[frontend] convert optional query params with boolean value to bool

5 years ago

praiskup commented 5 years ago

I have not, you are right. Sorry for dumb note.

Re-tried, and I see correct urls. There's still a nit:
config_opts['root'] = '_fedora-rawhide-x86_64'

Can this be fixed?

frostyx commented 5 years ago

I've updated the PR and implemented the suggested expand_repourls parameter.

1 new commit added

[cli] generate project_id on the client

5 years ago

1 new commit added

[cli] generate project_id on the client

5 years ago

frostyx commented 5 years ago

Re-tried, and I see correct urls. There's still a nit:
config_opts['root'] = '_fedora-rawhide-x86_64'

Can this be fixed?

Done

praiskup commented 5 years ago

Even though the repo ID doesn't seem to be very nice compared to the previous version (frontend now has precisely defined repo name, see repo file generator), it seems to work well. +1.

praiskup commented 5 years ago

I've gone through the api end points, and I really think that the helpers.generate_build_config() is exactly the info copr mock-config could use. Would you mind if I had a look at using this as a followup?

This is absolutely awesome output, and it is not convenient that the API clients can not easily generate the same output:

[copr_base]
name='Copr repository'
baseurl=http://dev-coprbe.devel.redhat.com/results/praiskup/test-copr/fedora-30-x86_64/
gpgcheck=0
enabled=1
skip_if_unavailable=1
metadata_expire=0
cost=1
best=1

That said, there's /build-chroot/build-config/<int:build_id>/<chrootname> endpoint, but that somehow artificially enhances the output of generate_build_config, which would otherwise exactly fit our needs.

Edited 5 years ago by praiskup

frostyx commented 5 years ago

Even though the repo ID doesn't seem to be very nice compared to the previous version (frontend now has precisely defined repo name, see repo file generator), it seems to work well. +1.

How so? The mentioned helpers.generate_build_config() gets the so-called project_id from copr.repo_id which is

@property
def repo_id(self):
    if self.is_a_group_project:
        return "group_{}-{}".format(self.group.name, self.main_dir.name)
    else:
        return "{}-{}".format(self.user.name, self.main_dir.name)

which should do exactly what I've implemented as a jinja filter.

praiskup commented 5 years ago

See the difference [http_copr_be_cloud_fedoraproject_org_results_praiskup_test_fedora_rawhide_i386] vs [copr_base] as set by generate_build_config.

I was curious how copr-rpmbuild generates the mock config file, and it led me to generate_build_config(); that (except for others) sets id/url/name triplet. That function was invented before for the purpose of copr mock-config.

frostyx commented 5 years ago

Closing this PR in favor of PR#450. We all agreed that it is superior to this PR.

Pull-Request has been closed by frostyx

5 years ago