Thank you for this PR/suggestion. At a first glance, it seems very similar to the make_srpm method for SCM source type and I am curious about the added value here. Also note that there will raise an issue with auto-rebuilding for this Custom Source Type. With SCM, we compare clone URLs in the incoming event with what's configured for a package to know if the event relates to the package or not. That approach is not possible here (due to missing Clone URL info). The last objection is that it is unclear what should be the output of the script.

I am curious about the added value here.

This method will work for every package out there, without ever touching existing
git repo or creating new one, just for the CI case.

.. That approach is not possible here (due to missing Clone URL info).

With this method, you need to put 'git' into minimal buildroot dependencies, and you
are responsible for cloning the repo yourselves.

The last objection is that it is unclear what should be the output of the script.

That's something which needs to be properly documented, but the output of this
script is content of %_sourcedir (tarball + spec file + maybe patches).

This method will work for every package out there, without ever touching existing
git repo or creating new one, just for the CI case.

Creating a new repo just for CI case is, I think, fine.

With this method, you need to put 'git' into minimal buildroot dependencies, and you
are responsible for cloning the repo yourselves.

You probably haven't read or misunderstood my argument about auto-rebuilding.

Creating a new repo just for CI case is, I think, fine.

I agree that it about preferences, but I prefer not to create additional repo and
not touching upstream repos or as minimally as possible.

You probably haven't read or misunderstood my argument about auto-rebuilding.

You are right. Can you elaborate? Why you need to compare clone urls?
The web-hook is not bound to particular package rebuild?

Ah, that's this (and sort of blocker here): https://bugzilla.redhat.com/show_bug.cgi?id=1344191

Please take another look at this WIP, there's big TODO list, but at least to understand the workflow idea, feedback needed.

Please don't add some extra command-line tools to the copr-rpmbuild package. All the functionality needed to implement the custom method should be present in the CustomProvider below. With this "concept" of extra script, copr-backend needs to know the build type (srpm/scm/custom) to invoke the right tool. It currently does get that information but it probly won't in the future to minimize data transfer between frontend and backend. It will only get what's important to schedule/allocate the job properly (arch, owner, task_id). Also, the backend would need to parse the source_json structure, we want only builder doing that.

Oh, I see you are invoking this script from the CustomProvider. I will read the whole thing first.

The actual task (job definition for the srpm build) needs to be fully specified by task["source_type"] and task["source_json"]. All the required information to build the srpm should be placed there.

Why are you creating another "local" executable. Is it possible not to do that?

You shouldn't need to obtain the mock config. The frontend's chroot build config (that gets translated into the respective mock config on builders) is only relevant for rpm builds, not for srpm builds. If you want a user to be able to provide a mock configuration (like the builddeps), then this configuration should be fully specified in the custom package or build definition (unless being always the same) and also be dedicated for the srpm build itself. The rpm build afterwards should follow the rules of the other rpm builds. Why? It's according to the already existing source types and the way srpms are created for them. I think the same rules should apply for all the dynamically created srpms.

I can keep this for my own debugging purposes locally, if that matters, any consequences out of curiosity? My "pros" is that it is just convenience wrapper which really simplifies testing of the software directly from git repo...

You shouldn't need to obtain the mock config. The frontend's chroot build config (that gets translated into the respective mock config on builders) is only relevant for rpm builds, not for srpm builds.

This is somewhat crucial motivating point for me though. I need to be able to on-demand update software in chroot, and I really want to let that affect the srpm build -- e.g. if I want to devel some universal web-hook-parser library, going through official fedora repos would be too long shot. Typical usecase for me: I may well try to "fix" or develop autotools packages only in my copr repo (which are used to produce tarballs through custom method ...).

If you want a user to be able to provide a mock configuration (like the builddeps), then this configuration should be fully specified in the custom package or build definition (unless being always the same) and also be dedicated for the srpm build itself.

The 'builddeps' parameter is dedicated to srpm build only, however... I don't take that as "configuration", but rather step to do to get the srpm built (maybe instead of specifying biuld-deps, I could specify procedural "root-prepare-command").

If you speak about "additional_repos" or "additional_packages" "configuration" --
configured per-project/per-chroot, it really is mock configuration I requested globally to build against -- and thus I want to prepare the srpm against that configuration, too.

The rpm build afterwards should follow the rules of the other rpm builds. Why?
It's according to the already existing source types and the way srpms are created for them. I think the same rules should apply for all the dynamically created srpms.

I guess I am missing the point here, can you please elaborate?

The point is that I need the copr id in question; to generate appropriate mock config.. Do you suggest to hard-code this once more into source_json?

Well, there is the provider interface and you should stick to it. Otherwise, you are breaking the interface, which is bad. It's as simple as that.

Does that mean "yes"? :) I don't feel it is clean. There's alternative option to pre-generate the mock configuration in advance -- before calling the provider.. but it would just complicate things.

Fwiw, consider that you develop /bin/rpkg... and you want to build it in copr and check that srpm builds fine with the re-built versoin --- for this reason I think wrapping other methods into mock chroot job would be useful, too.

Of course, we can also change the providers' interface... (is that what you meant)? Btw. that's why ask for advice since I have no significant preference...

We don't want to change the provider interface. source_type and source_json fully define the srpm build and any new method should stick to it. I am afraid you will need to update your requirements on this new method.

We don't want to change the provider interface. source_type and source_json fully define the srpm build and any new method should stick to it

Agreed. Though additional provider.prepare(task) call (usually unimplemented wouldn't be that bad).

I am afraid you will need to update your requirements on this new method.

So how? If you agree that I need that information, I'm interested to see what requirement should I update.

The requirement that chroot (and project) settings in COPR configuration influence the srpm build. There are two good options even without that. First is to extend the make_srpm method and give it the third argument: payload. In the make_srpm script, you can do everything you need to. And note that you can have just one Git repository with srpm generation scripts for all your maintained packages. For that you can use the subdirectory setting to invoke the correct script on the builder (i.e. you set the subdirectory for a given package to point to the directory where the respective srpm generation script can be found in your repo). The second option is giving structure to your custom script with one of the sections being e.g. "additional packages".

The requirement that chroot (and project) settings in COPR configuration influence the srpm build.

It is certainly what I want, however - same as I want to build (other) rpms against the buildroot, I want to build the srpms the same way. Please don't make me to relax this particular requirement :-/ that was something I was wishing about long time before I started doing the work and it is driving my work here :-)

I see the issue here (getting mock profile config), but nice solution will come with #185, can we agree on that? Until then, let's consider this WIP only. Btw., koji devels - after many years - now deal with the same issue (how to generate the mock config for specific target properly), while they realized that they want to generate slightly different mock config on fedora than on rhel (dnf vs yum issues).

nice solution will come with #185,

I meant #179 (I can not edit the comments, for some reason).

Sorry, but chroot and project settings are only intended for rpm building. It's already done like this for the other options to dynamically create the srpm in COPR. There are two good options to make your use-case possible so, please, stick to one them. In https://pagure.io/copr/copr/pull-request/179, you are just forcing a bad coding practice.

Sorry, but chroot and project settings are only intended for rpm building.

Who has/had this intention? It doesn't feel correct ...

It's already done like this for the other options to dynamically create the srpm in COPR.

How this is related to the new method - we can design/define/document anything differently in the new method, can't we?

In https://pagure.io/copr/copr/pull-request/179, you are just forcing a bad coding practice.

Self-confident, IMPOV statement which makes me sad again. Let that on the random reader where the true is.... but in #179 I was trying to only find a way to de-duplicate some efforts. You know how much time we spent on finding the best way towards this... The cause for all the words here is that some code (done with good, long-time vision by me) was re-implemented instead of re-used&&fixed (this was laziness, in the better case), and left behind giving inconsistent output. Now I need to use that code once more, without re-implementing, and you say that I'm spreading a bad coding practices.

Back to the topic. I'm concentrating quite some time on the I need statements, to let you better understand (I was told that nobody can say that I don't need something, if I clearly define what my needs are.. but it seems it doesn't work!). Something more about the vision.. I need a method where:

1. I can do anything (turing-complete thingy) to build the sources
2. I don't imply 'root' powers for the scripting (e.g. for installing packages)
3. I don't enforce anything (toolset (e.g. /bin/make, git), srcdir layout, hook caller, etc.)
4. I want to be able to update the tooling used to build the sources (say experimental autotools to build the latest tar's git)
5. I want to do this ^^^ as if I did this on my box (first install build dependencies, and then run something "safely" - without a root access).

I don't want to loose 4. while I don't want to regress to make_srpm with 2. and 3. having 5. in mind. Does this make sense? What's do you suggest to have 4. in with good practices?

Note that with make_srpm, you can do:

1) install all the packages from Makefile
2) drop root privileges
3) call an external bash script that will do the building

While in COPR, the Makefile is invoked to install the deps as well as call the script, on your local machine, you can just invoke the script directly to do the job and you don't need any special tooling except bash.

Or with the second option that I was proposing - the structured custom script with sections, you can have e.g. script that looks like this:

   chroot: "fedora-27-x86_64"
   additional_packages:
    - zsh
   additional_repos:
    - somerepo
   script_path:  "build_srpm.sh"

This snippet can be then into a mock config for a chroot in which the required command is invoked. You need to prepare some nice tool that will do this (mock config generation, chroot spawning and the script invocation) so you can say 3 is not satisfied but apart from that everything seems to be satisfied (and you will also be able to call build_srpm.sh directly on your machine).

So both options are very nice and I'll be happy if you contribute into implementation of any of those. Thank you very much!

• This snippet can be then translated into a mock config

I see your point in the "declarative" way of thinking, and that make_srpm. That's fine metod, but it is completely different topic than what I'm trying to achieve. My previous comment says it all.

Web-UI preview: http://praiskup.fedorapeople.org/Screenshot_20180116_093950.png

If I understood @clime's POV right, using the project's mock-configuration for RPM builds and only for custom srpm method (not for other methods) would be misleading for users, while implementing it globally also for all other methods would be too complicated. So the proposed solution by @clime is additional (optional) "textarea" with additional repositories (where the project copr:// url can be specified, too) in the proposed web-form, and in the commandline API.. This sounds good to me, but please allow me to solve this point (the point 4. in my use-case/motivating description above) later after this PR, is it OK? And please correct me if I'm wrong with something.

If I understood @clime's POV right, using the project's mock-configuration for RPM builds and only for custom srpm method (not for other methods) would be misleading for users, while implementing it globally also for all other methods would be too complicated. So the proposed solution by @clime is additional (optional) "textarea" with additional repositories (where the project copr:// url can be specified, too) in the proposed web-form, and in the commandline API.. This sounds good to me, but please allow me to solve this point (the point 4. in my use-case/motivating description above) later after this PR, is it OK? And please correct me if I'm wrong with something.

I guess it's completely alright.

Basic documentation is added. I just noticed there's missing new cli command for custom build, are there any other major design issues so I can squash the commits and finalize?

I don't think we need to explain to users the reasons why it is designed the way it is in the documentation. Documentation should only contain the information to make something work - not design-justifications. In other words, you can just say:

"The script is run under unprivileged user and any build requires for the script are provided as a list in an additional input field."

You seem to be comparing this method to the make_srpm method. In fact, this paragraph just sounds like "My method is better because...". I would recommend writing it in a more neutral manner.

Except the one comment on documentation, looking good at a first glance. I will do a more thorough review during the day.

You seem to be comparing this method to the make_srpm method. In fact, this paragraph just sounds like "My method is better because...". I would recommend writing it in a more neutral manner.

The purpose of the statement is really different. I just answered the questions we discussed before on the meeting; and I really want to make sure that this design is intentional, and convince the user that this isn't going to be changed so there's no reason to file a bug "give me a root access..".

Except the one comment on documentation, looking good at a first glance. I will do a more thorough review during the day.

To not waste your time, let me please cleanup the PR first. I'm now interested in the first-glance issues... e.g. I haven't created much testing yet. Is there something which I have to write tests for? I was thinking about tests for rpmbuild (the new script), but that requires "mock" permissions (basically that implies root), so that's definitely not usable for %check time... :-/ thoughts?

You seem to be comparing this method to the make_srpm method. In fact, this paragraph just sounds like "My method is better because...". I would recommend writing it in a more neutral manner.

Even if so, I would say that it's fine. Both methods have the same goal and provide a different solution. Naturally, there are advantages and disadvantages for both of them, so I think that it would be just fine to compare and motivate people to use them (both make srpm and custom) because of their advantages.

But I honestly can't see any trash talk about other methods or any attacks on them. Maybe just the "(btw. building SRPM under root is equivalently unfortunate idea as building RPMs under root)." which I believe was not intended as bashing make_srpm, but rather a general statement.

You seem to be comparing this method to the make_srpm method. In fact, this paragraph just sounds like "My method is better because...". I would recommend writing it in a more neutral manner.

Even if so, I would say that it's fine. Both methods have the same goal and provide a different solution. Naturally, there are advantages and disadvantages for both of them, so I think that it would be just fine to compare and motivate people to use them (both make srpm and custom) because of their advantages.
But I honestly can't see any trash talk about other methods or any attacks on them. Maybe just the "(btw. building SRPM under root is equivalently unfortunate idea as building RPMs under root)." which I believe was not intended as bashing make_srpm, but rather a general statement.

The docs should contain description needed to make something work for a user. Anything else is then just confusing.

looks like there is copy-paste bug here.

But I honestly can't see any trash talk about other methods or any attacks on them. Maybe just the "(btw. building SRPM under root is equivalently unfortunate idea as building RPMs under root)." which I believe was not intended as bashing make_srpm, but rather a general statement.

I would bet that from the outside this doesn't feel non-neutral or attacking the make_srpm method -- but I agree that we are able to compare, so I'll remove that. Originally when writing this, I had the %(bin sh) macros in mind (and the reasons for srpm building movement from dist-git to builders), rather than make_srpm method.

I don't think you can currently use the other /webhooks/github/ and /webhooks/gitlab/ entry points with the custom source type because of PackagesLogic.get_for_webhook_rebuild "filtering" (which is done by scanning source_json for a particular clone url).

Looks like another copy-paste bug here. Also below.

I tried both of the examples: trivial and simple and none of them work, which is kind of funny given the names. Apart from providing working examples, please, provide beaker tests.

I have some additional minor notes to the code but given the reload time after each comment, I think I will save them for now.

Please, don't forget to implement copr buildcustom subcommand (and yes, it should have this stupid name).

• copy paste bugs should be OK now
• the copr buildcustom command added
• web UI now has Build form for Custom method, too
• how the build form looks like now (@msuchy please take a look) https://praiskup.fedorapeople.org/Screenshot_20180124_184936.png
• the examples are now fixed, should be working
• I now explicitly documented that only custom webhook works for custom source method

Please have a look. I need to have a look at the beaker-tests now ...

how the build form looks like now ( @msuchy please take a look) https://praiskup.fedorapeople.org/Screenshot_20180124_184936.png

The text in the script area is very difficult to read and understand. What if I want to put there just...

 #!/bin/sh
 pyp2rpm -t epel6 motionpaint > motionpaint.spec

I would prefer only something really simple there, like one line saying:

# write a shell script that generates spec and sources...

That line says what's expected much more clearly.

That info below 'Result Directory' field:

 default= . ; the directory is resolved relatively to script's current working directory

That's just really cumbersome description. Especially the default=.; part. Also...shouldn't it be just saying:

(i) path relative to the script location

And into the field itself, you should add "Optional - " as we do it for other methods. You don't even need to say that default is .. That just comes naturally. I don't want to be figuring out descriptions for you but there is too much information there. Please, make the interface nicer.

You don't need to say everywhere that it's "mock chroot" where the script is evaluated. Nobody cares about it. It should be say just "chroot", not "mock chroot". You are unnecessarily tying it down to a particular implementation that we use today (and probably will be using for some time). It could also be evaluated in a systemd container or dedicated virtual machine and it shouldn't matter. This is quite important point. Chroot in COPR simply means os-version-arch triplet. We could also use word "target". How we implement building for a given "target" is our thing. E.g. here:

(i) packages to be preinstalled into mock chroot before script is run

"into mock chroot" part is completely redundant there. You could just say

(i) packages to be preinstalled before the script is run

or

(i) those packages will be installed into the script's environment before its execution

or even better:

(i) packages that the script requires for its execution

The problem with this approach for describing things is that is that if one thing changes, lots of other things need to change as well. That's not good.

I don't know why you changed "fedora-latest-x86_64" to "The latest branch Fedora (x86_64)". That's much worse especially because in the info line below, you again describe it as "fedora-latest-x86_64". I would just stick to "fedora-latest-x86_64" and live with it.

That info below the script text-area:

(i) script to generate sources used to generate source RPM ...

That's not a good description.

I know this was intended to be checked by msuchy but I really need to comment on this as well. Before those changes were made, the interface was actually much easier to understand. I thought you would only add a single info line below the 'Chroot' select field describing what "fedora-latest" is. That would be sufficient. Please, just trim the descriptions down as much as you can. There is simply too much information.

buildgem? Could you please check for copy-paste errors again? Don't use copy-paste if you don't change what needs to be changed.

it's "buildcustom" command, not "custombuild" command.

Can you, please, drop any design justifications from the method description? This whole paragraph basically.

• resultdir ... where the script generates its output. By default, it is assumed to be current working directory of the script.

(Note that for this, you first need to specify, what's the current working directory for the script in the above description. Otherwise, you should say "By default, they are assumed to be generated to the same directory where the script is located")

Additional note: Any mention of "dist-git" is completely redundant and inappropriate here.

You probably meant owner/repo.git there. Otherwise, this description is very nice.

Very good job in this part.

I would be careful and just say:

This source type uses a user-defined script to generate sources (which are later used to create SRPM). This is again completely redundant mention of copr-dist-git.

source_json_fixup. That's not a good name. Not for method and not even for its arguments.

It should be:

postprocess_source_json(source_json, source_type).

Also actually, this method should exist at all. The whole content should be in the Build's constructor because you are not going to need to call this code from anywhere else. And the if not 'chroot' in source_dict...that value for chroot should be generated by input form for the custom method, not here.

You could invoke just rpkg srpm there which does all those macro settings.

log_cmd method that does invoke subprocess....

soo. self.chroot attribute exists in the object only if 'chroot' in source_json...That's not correct way code in python. Especially, given that you use self.chroot later in produce_srpm.

Please, rather use:

self.chroot = source._json.get('chroot')

That also applies to the lines below.

The variable (and argument) naming should be the same for build and also the package methods.

I generally like this but there are many minor things that should be fixed. Also, I am not sure why the script is supposed to produce sources and not srpm directly. It would simplify description at lot of places but okay, I guess, it's a valid option. Anyway, thanks for you work here. I can already see now that it's becoming useful (and it even doesn't exist yet).

I just noticed that the srpm is created outside of the chroot. Is that really what we want? What if a user wants to pass a certain --define to rpmbuild (before I was imagining that he/she would do it by creating ~/.rpmmacros with the desired macros but this is not possible).

hm, -1, but if that's the blocker ...

-1, rpkg is not what I need (it does too much, I want low level tool if any), and this ideally should go away entirely (there are no technical reasons to even create source rpm).

I know you like writing, but saying "source_json.get(key) is cleaner" would be much firendly ;)

Thanks a lot the review. Looks really better now: http://praiskup.fedorapeople.org/Screenshot_20180125_174013.png

The last version: http://praiskup.fedorapeople.org/custom-method-latest.png

As for srpm vs. raw source generation...The problem is that we are generating srpms everywhere else and it's unlikely to go away. It would just really make things easier to explain. And you would also allow people to alter the srpm building process itself, which is something they might want to do (i.e. they will experiment with the build script by always changing just one --define value in the rpmbuild command line, then submitting build, changing again, then submitting build, etc.). I would like to discuss this again because I think you are thinking way too big here.

http://praiskup.fedorapeople.org/custom-method-latest.png

Yes, this is very good. I would go back to "fedora-latest-x86_64" in the select field or won't be using the word "branched" but "stable" or "released" instead (because lots of people probably won't have an idea why it is called "branched") but otherwise nice.

What about the latest "branched" Fedora for other archs? You should cover all the archs here.

Well, rpkg actually does exactly what you are doing here if all the sources are already present (which is the case here).

It looks good. You haven't dropped that "why this design" paragraph that really shouldn't be present in a user documentation but I don't mind it that too much given that this PR is now in a pretty good shape. But really...if it's good, then you don't need to explain why it is good. Anyway, thanks. I am now looking forward to see the tests (I need to additionally test out the examples).

It looks good. You haven't dropped that "why this design" paragraph that really shouldn't be present in a user documentation but I don't mind it that too much given that this PR is now in a pretty good shape. But really...if it's good, then you don't need to explain why it is good.

It is not about describing that it is good ... it is really "why"...

As for srpm vs. raw source generation...The problem is that we are generating srpms everywhere else and it's unlikely to go away. It would just really make things easier to explain.

... e.g. here, if you require that user has to do the 'rpmbuild -bs', that script is not anymore (easily) portable to Debian, Gentoo, etc. (described in the "why" docs). Anyway, script-API is different from metdhod-API (and the method genertes SRPM, so it doesn't diverge).

That reminds me -- I didn't touch your hint aboout this before; yes generating the srpm on host is desired, because it is more likely that srpm generated on host will be "importable" on dist-git machine (SRPM generated in fedora-rawhide-x86_64 can have too new feature to import correctly; epel-6-x86_64 can be theoretically too old).

Well, rpkg actually does exactly what you are doing here if all the sources are already present (which is the case here).

At least I now learned that the rpm is configured by .rpmmacros, which is (at least for every use-case I ever had in mind) unexpected side-effect... What if we created "tarball" instead of src.rpm, in the end?

As for srpm vs. raw source generation...The problem is that we are generating srpms everywhere else and it's unlikely to go away. It would just really make things easier to explain. And you would also allow people to alter the srpm building process itself, which is something they might want to do (i.e. they will experiment with the build script by always changing just one --define value in the rpmbuild command line, then submitting build, changing again, then submitting build, etc.). I would like to discuss this again because I think you are thinking way too big here.

If the SRPM is generated with some --define, it is anyways extracted on dist-git side, and imported... We really use the SRPM as transport format, and that's huge benefit of copr which would disappear with dist-git machine was not used as is. Now, once imported, users are sure that they bulid byte-by-byte the same content in all chroots...

Yes, this is very good. I would go back to "fedora-latest-x86_64" in the select field or won't be using the word "branched" but "stable" or "released" instead (because lots of people probably won't have an idea why it is called "branched") but otherwise nice.

I'll take a look at this, ok. The "branched" is really correct word in fedora world..., but latest is fine, too.

I'm not convinced that other arches are needed, should I add them now, or can we wait for explicit request?

If the SRPM is generated with some --define, it is anyways extracted on dist-git side, and imported... We really use the SRPM as transport format, and that's huge benefit of copr which would disappear with dist-git machine was not used as is. Now, once imported, users are sure that they bulid byte-by-byte the same content in all chroots...

Except that the content of external repositories might be completely different. You are overestimating the value of the feature. But even if that was true, there is another way to assure it (to keep the built srpms, which we already do). But I don't want to go into this now. At the moment, copr-dist-git has still its value at the moment.

Except that the content of external repositories might be completely different. You are overestimating the value of the feature. But even if that was true, there is another way to assure it (to keep the built srpms, which we already do). But I don't want to go into this now. At the moment, copr-dist-git has still its value at the moment.

Actually....we could just send every srpm (even uploaded ones) to copr-backend and store it there and then use srpm_url attribute for rebuilding. This is actually the good way to do things :).

Except that the content of external repositories might be completely different. You are overestimating the value of the feature. But even if that was true, there is another way to assure it (to keep the built srpms, which we already do). But I don't want to go into this now. At the moment, copr-dist-git has still its value at the moment.

Actually....we could just send every srpm (even uploaded ones) to copr-backend and store it there and then use srpm_url attribute for rebuilding. This is actually the good way to do things :).

We can drop the copr-dist-git redundancy and use it to deploy our own writeable DistGit! \o/

Yes, this is very good. I would go back to "fedora-latest-x86_64" in the select field or won't be using the word "branched" but "stable" or "released" instead (because lots of people probably won't have an idea why it is called "branched") but otherwise nice.

I'll take a look at this, ok. The "branched" is really correct word in fedora world..., but latest is fine, too.
I'm not convinced that other arches are needed, should I add them now, or can we wait for explicit request?

Please, add them now. There is no reason to "prioritize" one arch over another.

Actually....we could just send every srpm (even uploaded ones) to copr-backend and store it there and then use srpm_url attribute for rebuilding. This is actually the good way to do things :).

I consider the actual dist-git very useful as I browse cgit html pages all the time (mostly internally, Fedora copr has issues with cgit most of the time unfortunately). It is invaluable to see differences in specfiles between two successful imports (though yes, I wouldn't be against cleaning-up the lookaside cache).

We can drop the copr-dist-git redundancy and use it to deploy our own writeable DistGit! \o/

This is orthogonal feature, automatic imports can coexist with manual pushes; same as this is supported by fedpkg workflows. Off topic, sorry :-)

Except that the content of external repositories might be completely different.
You are overestimating the value of the feature.

I don't understand this, the content of imported dist-git should be completely the same, that's the point. But yes, that can be achieved different ways.... but...

But even if that was true, there is another way to assure it (to keep the built srpms, which we already do). But I don't want to go into this now. At the moment, copr-dist-git has still its value at the moment.

... this is possible, but note that srpms are not portable. If you build the srpm on f27 (currently on builders), you can not be 100% sure it is importable in epel-5-x86_64 (some copr instances might still support this, I have a request to support rhel-4-x86_64 chroot actually). Anyways, I know that last time we discussed this the conclusion was to go through survey first .. about "how much automatic imports collide with manual pushes".

I consider the actual dist-git very useful as I browse cgit html pages all the time (mostly internally, Fedora copr has issues with cgit most of the time unfortunately). It is invaluable to see differences in specfiles between two successful imports (though yes, I wouldn't be against cleaning-up the lookaside cache).

I believe we will be able (if required) to provide the means to maintain the dist-git imports by providing post-build hooks so that you can plug-in the distgit import mechanism into copr-frontend after a srpm build is completed. It's likely to disappear from the core, however, so that we can utilize the freed up resources in Fedora (the copr-dist-git machine) for something else (writeable DistGit). The generated srpms will be kept on backend and pruned by prunerepo after some period of time. The colored diffs on specs that the cgit provides are nice but it's not a reason to keep copr-dist-git as a required feature. Seems rather like an optional thing right now. So...it's likely to become an optional feature, possibly implemented by a plugin. In this case, we will also need to rework copr-dist-git to react to post events (instead of polling copr-frontend periodically) to execute the import.

This is orthogonal feature, automatic imports can coexist with manual pushes; same as this is supported by fedpkg workflows. Off topic, sorry :-)

I am not sure how fedpkg supports that. Anyway, this is rather a server-related (not client-related) problem and I haven't seen any automatically imported packages on src.fedoraproject.org. Also it's not really orthogonal for me because we will be able to re-use the copr-dist-git machine for something much more useful. The current solution is not very economical.

Except that the content of external repositories might be completely different.
You are overestimating the value of the feature.

I don't understand this, the content of imported dist-git should be completely the same, that's the point. But yes, that can be achieved different ways.... but...

I was just saying that it's nice that you have the same sources (the imported content) but during the rpm build the versions of downloaded build dependencies might be different. Anyway, we will be keeping the built srpms, just probably not in their extracted form.

... this is possible, but note that srpms are not portable. If you build the srpm on f27 (currently on builders), you can not be 100% sure it is importable in epel-5-x86_64 (some copr instances might still support this, I have a request to support rhel-4-x86_64 chroot actually). Anyways, I know that last time we discussed this the conclusion was to go through survey first .. about "how much automatic imports collide with manual pushes".

I don't exactly see what you see with the importability problem of the built srpms. I believe that's practically a non-existent issue because the srpm binary format itself would need to be different across distro versions for this problem to occur. But...even if that was the case...why is it important that our srpms cannot be imported on a rhel-4 machine? I don't see it. I mean, I can see some kind of possible issues with this but I am not sure how practical they really are.

Or if you mean that the f27 srpms cannot be used for a subsequent rpm build in rhel-4 chroot, that I understand. Does it actually happen? if it is the case, we can first extract the stored srpm on host directly and then use the --buildsrpm feature of mock to rebuild the srpm for the given chroot so that it is compatible.

And yes, there was some survey. It doesn't need to conflict but it's also not a necessary feature to have on copr-frontend. It's not necessary because we can provide tooling in rpkg that will make the imports easy-to-do on command-line (basically it will be rpkg import <srpm_url>...maybe it's possible even now, I need to have a look. It will be on-demand importing. We can discuss it on next meeting. I am open to suggestions!

Or if you mean that the f27 srpms cannot be used for a subsequent rpm build in rhel-4 chroot, that I understand. Does it actually happen?

That's what I meant, yes. It is nitpicking though, I don't object against having srpm in place as it is now -- I'm just saying that if we sent extracted srpms (or rather tarballs) from bulider to backend/dist-git, that would be much cleaner. The custom method generates srpm, so I hope that is fine...

And yes, there was some survey.

Do you have the link to the discussion?

Btw., thanks for all the ideas, but I would rather went back to the PR issue, we can discuss this later.

I am ok with srpms.

Do you have the link to the discussion?

Nope, It was really just ear-to-ear info.

Btw., thanks for all the ideas, but I would rather went back to the PR issue, we can discuss this later.

Yup, sure. Waiting for the tests now ;).

PTAL

What was the reason to make new directory for 'test' builder? Also this doesn't really test builder only...if there are things like copr edit-package.

I originally thought that I'll provide spec file directly...

What was the reason to make new directory for 'test' builder?

Fixed.

Also this doesn't really test builder only...if there are things like copr edit-package.

The primary goal is to test things on builder. The side-effect is that it tests the cli and
frontend..

What was the reason to make new directory for 'test' builder?
Fixed.

I don't know what you have fixed but anyway...you shouldn't be spawning frontend and call methods against it in builder Regression tests. Those tests should only contain invocations of copr-rpmbuild with the desired input to be tested. You can use ~/copr/mocks to supply the input to copr-rpmbuild.

The stuff like copr edit-package-custom or copr build-package belongs to beaker-tests/Sanity. These tests that are running against dev instances by default but they can be configured to use e.g. the local docker stack.

Problematic things (summary):
- Vagrant is deprecated
- copr-mocks should be used to test copr-rpmbuild instead of spawning full frontend (related to this point, there is lots boiler-plate code that's not really needed)

I don't know what you have fixed but anyway...you shouldn't be spawning frontend and call methods against it in builder Regression tests.

Because?

• Vagrant is deprecated

Other Regression tests are using it (frontend). What obsoleted Vagrant? Where this is stated...

• copr-mocks should be used to test copr-rpmbuild instead of spawning full frontend (related to this point, there is lots boiler-plate code that's not really needed)

There's nothing more unsympathetic than copr-mocks to me :-(. What's the benefit in writing some code which can easily get de-synchronized with the real code of frontend? Are you able to help here with the fix (patch)? I agree with the boiler-plate code POV, but the design is unfortunate.. One should be able to specify what version of package I want to test (git latest version, stable version, released (fedora) version) plus what versions of particular components to test against (frontend, git/stable/released ...) and that's it.

I don't know what you have fixed but anyway...you shouldn't be spawning frontend and call methods against it in builder Regression tests.

Because?

Because it's very inefficient and you don't need to do it. copr-mocks will do the necessary job. Those tests will should be run automatically at some point and we cannot just spawn and provision some machine when it's not needed.

Vagrant is deprecated

Other Regression tests are using it (frontend). What obsoleted Vagrant? Where this is stated...

We now have the docker stack and I believe that's better maintained.

copr-mocks should be used to test copr-rpmbuild instead of spawning full frontend (related to this point, there is lots boiler-plate code that's not really needed)

There's nothing more unsympathetic than copr-mocks to me :-(. What's the benefit in writing some code which can easily get de-synchronized with the real code of frontend? Are you able to help here with the fix (patch)? I agree with the boiler-plate code POV, but the design is unfortunate.. One should be able to specify what version of package I want to test (git latest version, stable version, released (fedora) version) plus what versions of particular components to test against (frontend, git/stable/released ...) and that's it.

copr-mocks has perfectly fine design for its job (testing separatelly copr-dist-git and copr-backend, and now copr builder as it looks. It provides the frontend's /backend/ intefaces with just minimal implementation. Test tasks are loaded from a file and published at the respective interfaces and fetched by the other services that are being tested. Sure if you want some help, let me know. /backend/get-srpm-build-task doesn't seem to be implemented yet in copr-mocks. Feel free to implement it or let me know if I should do it.

I would suggest you to drop Vagrant from the testsuite, then. Even though I am
not huge fan of docker, waiting for cri-o and friends... Before done so, it
sounds like I have to go and debug why the docker-stack doesn't work for me (it
failed for me last time after 20minutes of building something). Can we solve
this later?

Beaker tests are unfortunately not about performance (zero paralelism), and I
see zero additional value in testing of the components separately. More, I see
a lost opportunity to test more parts of the real code.

Sure if you want some help, let me know.

How much work would you spent on editing the proposed tests to work according to
your ideas with copr-mocks? Because I would really appreciate the help here...
(I'm not fan of the copr-mocks stuff, and hardly will be in the future). I'm
not really sure that you'll get to have some significant speedup, yes the
frontend build time... but otherwise.

Please consider my offer with help (not a prerequisite for this PR, though) with
optimizing the tests so they work also without copr-mocks. So we can test real
components against real components. By that I mean -- we should really make the
deployment of copr stack a matter of seconds, e.g. in docker for now. E.g. by
building copr-{frontend,backend,dist-git} docker images, and pushing them to
fedora registry on a regular basis... so, if that's enough, people don't have
to waste the time with building the images.

Beaker tests are unfortunately not about performance (zero paralelism)

Not sure what you mean. I run those tests during production deployments and I certainly don't want to wait 30 mins until something gets provisioned that is not required in the tests (because it can be replaced by copr-mocks that are started instantly).

More, I see a lost opportunity to test more parts of the real code.

In "builder" regression tests only the builder should be tested not to get obfuscated results.

How much work would you spent on editing the proposed tests to work according to
your ideas with copr-mocks? Because I would really appreciate the help here...

Well, I offered you to implement get-srpm-build-task into copr-mocks. I didn't say I will be doing all the other changes for you. I will be on IRC next week, so please, feel free to ping me with questions.

Please consider my offer with help (not a prerequisite for this PR, though) with
optimizing the tests so they work also without copr-mocks. So we can test real
components against real components. By that I mean -- we should really make the
deployment of copr stack a matter of seconds, e.g. in docker for now. E.g. by
building copr-{frontend,backend,dist-git} docker images, and pushing them to
fedora registry on a regular basis... so, if that's enough, people don't have
to waste the time with building the images.

Ye, please, let's focus on finishing this PR with the current test infrastructure that uses copr-mocks.

If it can wait we also do a quick team coding session at the office where I can give you more specific how-to on the required changes. It shouldn't be that hard but in case you are unclear about them, then this could help as well.

Not sure what you mean. I run those tests during production deployments and I certainly don't want to wait 30 mins until something gets provisioned that is not required in the tests (because it can be replaced by copr-mocks that are started instantly).

Good question is why it takes 30 minutes to provision. We should get the provision
matter of seconds or at most few minutes (openshift-like speed).

Well, I offered you to implement get-srpm-build-task into copr-mocks. I didn't say I will be doing all the other changes for you. I will be on IRC next week, so please, feel free to ping me with questions.

copr-mocks is not a something I'll ever use, and it is against my belief.

In "builder" regression tests only the builder should be tested not to get obfuscated results.

This opinion deserves highlight. So if the test for rpmbuild finds an issue in cli or frontend, it is obfuscating... :-) No, taking care of copr-mocks is something I don't want to waste time on.
It just delays solution of real problems (slow startup time).

Before done so, it sounds like I have to go and debug why the docker-stack doesn't work for me (it failed for me last time after 20minutes of building something).

@praiskup, there is a known issue of frontend build failing because of docs generation. It was fixed in 8f7b53b5. Hopefully, it will help.

@frostyx thanks for the help, but hacking on copr-mock is like writing emulator for something which we can trivially use without emulation. Feel free to work on that, but I wont do that.

I see no major issue with the tests in this PR, but feel free to merge only the first two patches (without beaker tests).

@frostyx thanks for the help, but hacking on copr-mock is like writing emulator for something which we can trivially use without emulation. Feel free to work on that, but I wont do that.
I see no major issue with the tests in this PR, but feel free to merge only the first two patches (without beaker tests).

This PR is unfinished because satisfying tests were not yet provided :( and it is, therefore, unsuitable for merging.

Here are the options that you have to fix the PR:

1) keep only basic set of Sanity (integration) tests (that will go into beaker-tests/Sanity directory) that create/edit/build custom packages through copr-cli interface and that also test the copr-cli buildcustom command. Take e.g. look at beaker-tests/Sanity/copr-cli-basic-operations/upload_authentication.sh. So if you deliver just beaker-tests/Sanity/copr-cli-basic-operations/custom.sh (something that can be easily run and tested) and nothing else then it's also good.

2) Fix the builder tests (beaker-tests/Regression/builder) so that they don't spawn any unnecessary copr machines. The regression tests are always focused to test just one particular COPR component in separatation and they should do nothing else than that (we have integration/Sanity tests to test COPR stack as a whole). We have copr-mocks package to read build tasks from a file and supply them to other components as copr-frontned would normally do. These tasks, read from a file, represent the test data set. You can use this package or you can also implement what was described in https://pagure.io/copr/copr/issue/102 and test copr-rpmbuild directly from command-line without the need for external http interface providing the tasks.

I am ok with any of those two options and the first one should be quite easy. You can also implement both of them if you feel like it. Thanks for your work so far. I hope we can get this to the finishing line.

I must say that the division of tests to two categories -- Sanity and Regression -- is wrong, and I don't think that's how the beakerlib is normally used (try rhpkg tests <PACKAGE> internally).
We should be able to run both kinds of tests against both CI provisioned copr stack and development/production stack. So 1) sounds like you want me to write something I'm not able to check that it works easily, and 2) is no-go with copr-mocks (reasons said before). I can add more tests related to copr-sources-custom as suggested in #102. But that would mean that I would have to drop all the coverage I wasted a lot of time on.

This hostile environment makes me tired, and I'm running out of energy. So if you wan't to drop the tests (you claim them bring just obfuscation), please do it yourself. I'm not going to rewrite tests against docker, because 'vagrant up' is used on two more places in the beaker-tests directory -- and forcing me to not use it is your random whim. I really think that the tests I spent time on bring something good, and thus I'm taking the 3) option -- waiting for your help with beaker-tests. Plus, count with my help I mentioned before (solving the slow-start problem).

1) sounds like you want me to write something I'm not able to check that it works easily

It's quite easy to check it. Spawn docker instance. Then you need to update ~/.config/copr in your testing environment to point to the docker intance. And you should be done. You can now test against docker instance.

2) is no-go with copr-mocks (reasons said before).

Ye, none of the reasons I really accept. Regression tests were made to tests components in isolation on their published interfaces. copr-mocks allows it and it also allows to formally specify tasks to be run. It's a good and useful tool.

I won't be persuading you to fix the PR. I politely asked you several times to fix the problems in the PR and you haven't done it regrettably. I am not sure why I should be finishing the PR for you. If you ping me on IRC or personally I can help you if you have any troubles.

I've looked at the tests and decided to solve the issue by re-writing them as Sanity tests. Can you guys check the patch, whether it is alright?
https://frostyx.fedorapeople.org/pagure/0001-beaker-tests-sanity-test-custom-method.patch

Then I would like to ask you @praiskup to rebase the branch against the master. I did it myself, but kinda sloppy. Also, I didn't remove the original tests.

Thank you @frostyx, looks fine. I'm OK to rebase now, should I @clime? (removing of the
tests I added, and keep only jakub's tests (under his name).

Thank you @frostyx, looks fine. I'm OK to rebase now, should I @clime? (removing of the
tests I added, and keep only jakub's tests (under his name).

Yes, please.

PTAL

LGTM. Thanks.