Note that the repo generation code was added into copr-rpmbuild because when we add -a option to enable additional repos in buildroot, then user can use copr://user/project syntax directly from command-line.

That's also why backend-url option was added into copr-rpmbuild's config, so that this shortcut is possible. Also priority settings was intended to be available on command-line for a user to use.

-a option is inspired by mockchain and it allows that same thing - to enable additional repos in mock's buildroot.

Note that copr-rpmbuild enables you to launch a build from url or from a Git repo without a previously created project on copr-frontend but you might still want/need to add an additional copr repos for the build into buildroot or potentially alter the priority of the added repos or tweak other build params (e.g. additional_packages aka buildroot_pkgs)

Note that you are changing name of an attribute in API3, which is a backward incompatible change.

This remains: config_opts['root'] = '_fedora-rawhide-x86_64'

Otherwise +1.

Note that you are changing name of an attribute in API3, which is a backward incompatible change.

I know. This is just a suggestion. Although I think, that in reality, it wouldn't affect anyone (what is the chance, that someone is using just build config from APIv3) and we can just do the change if we want to, we should establish some API change / deprecation policy in our documentation.

However, backward incompatible change doesn't have to be done here. I can put the additional_repos list back, mark it as deprecated and repos can be added next to it.

I know. This is just a suggestion. Although I think, that in reality, it wouldn't affect anyone (what is the chance, that someone is using just build config from APIv3) and we can just do the change if we want to, we should establish some API change / deprecation policy in our documentation.

I don't really know what to say to this. Doing incompatible changes like nothing is happening is not good idea in my opinion.

However, backward incompatible change doesn't have to be done here. I can put the additional_repos list back, mark it as deprecated and repos can be added next to it.

Why we can't just keep additional_repos?

Nobody reacted to my other points so to sum them up once more: this PR is regressing usage of the -a option. It would be good to have available both copr:// and the priority twiddling on the command-line.

Just to sumup f2f meeting today:
- we can keep additional_repos, and add 'repos' as a complement (backward compatibility)
- we can implement -a pretty easily even if we do this PR

I was thinking about it a little more and figured we don't actually need this PR, nor PR#435. We just need to use the original API for the copr mock-config feature, that means API1. So copr mock-config should just keep using API1. It doesn't need to be ported to APi3 as it is a deprecated feature as well as API1. We can remove them approximately at the same time.

copr mock-config doesn't need to go anywhere :-) and yes we need APIv3 fix. Please concentrate on good architecture decisions, and put personal things aside.

copr mock-config doesn't need to go anywhere :-) and yes we need APIv3 fix

Nope, we don't, there is no reason. Both copr mock-config and API1 are going away so it doesn't make sense to try to implement copr mock-config using API3. We didn't even need to that from start.

Both copr mock-config and API1 are going away

Being tired from your unfriendly style: I can't speak for API1 -- but copr mock-config can not go away only because you say so, period. I'll take care of that feature and I'll add tests, documentation and I'll promote that (in blog post, by example of its usage).

This PR has two issues:
- config_opts['root'] is broken
- we should keep additional_repos for compatibility, even though it is (in a form of list of urls) probably useless thingy

Otherwise it is good to go, thanks. I can see (+1 +1 -1) which is still good to go.

Being tired from your unfriendly style: I can't speak for API1 -- but copr mock-config can not go away only because you say so, period. I'll take care of that feature and I'll add tests, documentation and I'll promote that (in blog post, by example of its usage).

Pavel, could you, please, do it for copr-rpmbuild --dump-configs? That would be cool! As far as I remember copr-rpmbuild --dump-configs was agreed to be a sufficient alternative by our group and to have an alternative was a prerequisite to deprecate copr mock-config.

Anyway, this PR is good in sense, that it moves resolution of copr:// scheme from copr-rpmbuild back to copr-frontend. That's probably a good thing because we can keep copr-frontend as the only component that really knows where a copr yum repo is located.

Nevertheless, we shouldn't break backward compatibility by doing it and we should limit this PR only to fixing copr-rpmbuild and the API. As for fixing copr mock-config PR#452 seems to be the best choice.

This PR has two issues:
- config_opts['root'] is broken

I suggest 460d1beb as a solution. What do you think?

• we should keep additional_repos for compatibility, even though it is (in a form of list of urls) probably useless thingy

I put the additional_repos attribute back, so there is no break of backward compatibility anymore.

As far as I am concerned, 460d1be is not a PR fixing copr mock-config. It is a PR doing bunch of other unrelated things and by the way fixing copr mock-config.

We should fix copr mock-config simply by PR#452 and keep this PR as the improvement on the API and the handing of copr:// scheme.

Other thing is that repos would be better called additional_repos_expanded or additional_repos_config so that it is clear how it relates to additional_repos attribute.

Being pedantic, I'd rather not move the additional_packages down, just to make the diff nicer.

I personally don't see the config_opts['root'] as that important info so we'd necessarily had to add a new field here; I'm not strictly against this, but at least it should have better name than repo_id (there are many repo IDs in repos field).

For me, since copr mock-config (or any other third party "client" tool) knows well what project and chroot he is working with, I'd be confident that the root name could be generated on clients, and that not all client tool needs to use the same root name (path on the disk).

That said, if we used mock_root_dir, I'd be probably +1 for this. It would be small synchronization step towards doing everything on one place.

This rant is unrelated to this PR, but the work with task_id is a bit ugly magic :-) that would deserve some code-cleanup definitely.

Otherwise, I'm +1 (the two notes above are not blockers).

On Thursday, October 25, 2018 8:36:10 PM CEST clime wrote:

clime commented on the pull-request: Fixcopr-cli mock-configafter switching to APIv3 by preprocessing repos on frontend that you are following:
` We should fix copr mock-config simply by PR#452 and keep this PR as the improvement on the API and the handing ofcopr://` scheme.

PR 452 doesn't make sense (regressing back to api v1). I proposed that as
a work-around at the beginning, but this PR is really much better and
cleaner (using APIv3 is definitely a good thing her, for cli but mostly
for the APIv3 itself).

Other thing is that repos would be better called
additional_repos_expanded or additional_repos_config so that it is
clear how it relates to additional_repos attribute.

Well, the repos field doesn't really hold only the "additional_*" repos
(see generate_build_config() what we mean by "additional" term, but we
also provide a base repo there).

Also, for user it must really be confusing word -- very easily
"exchangeable" with the "External" repositories term).

Note that it is not really only "expanded" form, there is much more
semantic info about the repositories.

^^^ That said, I wouldn't be against repos_config, but still -- I think
that repos (or actually repositories is even nicer) dict would be much
nicer and expectable name. This would be the best IMO:

  repositories = [
    {
      id='...',
      name='...',
      baseurl='...',
      ...
    },
    ...
  ],

So we are as close as possible to dnf/yum naming and semantics.

No problem with being pedantic, but I would rather have the final code nicer than the diff and therefore have repos one line above additional_repos. Not with additional_packages in between.

I was about to fix it on the client side by adding project_id function from PR#435, but then I found out, that copr-rpmbuild is doing the same thing. It is a small piece of code, we can let both of them generate it by themselves, or we can add the field (I was not sure about its name). I think, that both solutions are fine.

Ack.

we can let both of them generate it by themselves, or we can add the field (I was not sure about its name).

I don't have huge preference here; just please use some better name than repo_id.

Note that project_id sounds like "standardized" copr term, but it is not (it is really only used for mock config generation). I'd be afraid that some people could misuse that field, and it would be "bounding" for us in future. Saying root_dir wouldn't "promise" that much to end-users.

Another note is that the s/@/group_/ is not needed at all for mock root purposes.

Note that project_id sounds like "standardized" copr term, but it is not (it is really only used for mock config generation). I'd be afraid that some people could misuse that field, and it would be "bounding" for us in future. Saying root_dir wouldn't "promise" that much to end-users.

Or maybe even mock_root_dir? I fine with both names.

Another note is that the s/@/group_/ is not needed at all for mock root purposes.

You mean the change of repo_id in models.py? That's just refactoring, it does the same as before. Sooo, it can be possible to have "@" in the mock root dir, but we already substitute it to "group_".

... root_dir
Or maybe even mock_root_dir? I fine with both names.

Actually, ... I wouldn't go with root_dir. Why should frontend know anything about mock or its rootdir. What about calling it just simply id? It is natural identifier for the project-chroot build config (the combination of project full name and chroot name). The fact, that it is used also for config_opts['root'] can be considered as a lucky coninsidence.

To sum this up: I need to change the name of repo_id and repos attributes and this PR is ready to go? Let's figure them out offlist, it is going to be faster.

Actually, ... I wouldn't go with root_dir. Why should frontend know
anything about mock or its rootdir.

This is good point, it is suboptimal.

What about calling it just simply id?
It is natural identifier for the project-chroot build config (the
combination of project full name and chroot name).

There are three pieces of information - owner & project &chroot - in a
pretty complicated format which we might or might not need to change in
future. For me, creating such "ID" would be unnecessarily bounding,
considering that it is anyways known in advance anyway (you need to know the
triplet to be able to form the right request to frontend).

The fact, that it is used also for config_opts['root'] can be considered
as a lucky coninsidence.

That's right, and it is IMO not very nice. As the discussion goes, I'd
prefer to let the config_opts['root'] on client tool, instead of putting
it into API.

Anyways, this is pretty minor. If you don't like to generate mock root
field on two places (or reuse it from common), I don't want to block this with more
wishes. So +1 for the actual diff, too.

As the discussion goes, I'd prefer to let the config_opts['root'] on client tool, instead of putting it into API.

Yeah, me too. At first, I thought it would be a good idea, but ... sry, for an unnecessary digression on it. I will put it to clients.

As the discussion goes, I'd prefer to let the config_opts['root'] on client tool, instead of putting it into API.

Yeah, me too. At first, I thought it would be a good idea, but ... sry, for an unnecessary digression on it. I will put it to clients.

Done.

@frostyx please split this into two PR - one with API and rpmbuild changes (I will merge it) and the other with copr-cli (which will not be merged)

@frostyx please split this into two PR - one with API and rpmbuild changes (I will merge it) and the other with copr-cli (which will not be merged)

I've rebased this PR and moved all the copr-cli changes out of it. The name of this PR "Fix copr-cli mock-config after switching to APIv3 by preprocessing repos on frontend" is now misleading, because it does not fix the mock-config anymore. Instead, it adds everything, that is needed to fix/implement it. Please take a final look on this.

I am going to open another PR with the copr-cli changes, that disappeared from this PR.

I was not on the meeting, but I guess I know where the additional burden (split of PRs) comes from... that was just useless step back because of individual's arbitrariness.

Otherwise I'm +1.

The controversial copr-cli changes were moved to the PR#466, so this PR only changes the copr-frontend and copr-rpmbuild which everyone agreed on last meeting and @praiskup in the comment above, so I am merging this.