Why did you change this?

Why is this getting dropped all across the file?

Again why?

Because there was no reason this needs to be passed through constantly all throughout the code: it can access the pagure_config itself, and in addition, the gitfolder might end up being entirely unused if it's a repo on repoSpanner. See the repopath function.

@ngompa the commit messages have a little more background on why. Please read.

https://pagure.io/fork/puiterwijk/pagure/c/38e2e801380dce169f752a1a4b179adc1dd5f9a2

Wut... No, why are you doing this? Why are we shelling out to git instead of using pygit2?

Please, no. :cry:

:broken_heart:

Because the transports in libgit2 (and thus pygit2) have no way to do client cert authentication.
And adding that there is a significant operation given their abstraction choices.

So, I'm going to be that guy. I don't think it's worth it to do this given that you already have to adjust libgit2 for repospanner support.

Significant operation or no, since your mechanism already requires changes to the library (and thus the Python binding), you should do it right and propose it upstream. There is no reasonable way I could, in good conscious, say this is okay to merge.

The BaseRunner hooks architecture change looks good to me, but I currently cannot say that it's a good idea to merge repoSpanner, given that it requires changing libgit2 and it's very clear that support for it is still haphazard (since we're now shelling out to git). It is patently a bad idea to have a patched libgit2 (with non-upstreamed changes!) as a requirement, especially since Pagure is being used by more than Fedora in more places and configurations.

So, sorry, but :thumbsdown:

Also, I'm going to be that guy about authentication and say you shouldn't have made it rely on cert-based authentication. In pretty much every other variation of this, I've seen either the use of storage replication (and thus, nothing is aware) or API token based HTTP(S) access.

We've gone through some seriously weird contortions to avoid supporting that in Pagure, but this is too high of a price to pay.

@puiterwijk Please break out your BaseRunner hooks architecture change into a separate PR.

Should it be PSEUDO instead of PSUEDO ?

Ah apparently both spelling can be used :smile: . Learned something today

Do note that pagure code is already shelling out to git for example here https://pagure.io/pagure/blob/master/f/pagure/lib/git.py#_1101 or here https://pagure.io/pagure/blob/master/f/pagure/lib/git.py#_1180, https://pagure.io/pagure/blob/master/f/pagure/lib/git.py#_1188 etc

@puiterwijk Please break out your BaseRunner hooks architecture change into a separate PR.

I would +1 this, it will be much easier to review the PRs.

@puiterwijk Please break out your BaseRunner hooks architecture change into a separate PR.

I would +1 this, it will be much easier to review the PRs.

While technically it would be possible, there is some common code that I added to the repoSpanner one that is also being used by the hook commit (e.g. the Project.repopath() function).
So, while it's technically possible to strip that common code out and separate these two PRs, that would also be a reasonably sized chunk of work.
So unless strictly required, I'd prefer if the review can be done by looking at the separate commits.

Thanks.

Pretty please pagure-ci rebuild

Hm, this means we are no longer failing if we create a project with a git repo already existing on disk. Not sure how I feel about this.

Basically, we've had cases where project got incompletely deleted or so, and when people would try to re-create it (or fork again) we would fail because we didn't clean up properly. This would now work (great in the case of a fork, less so in the case of a main project if the project deleted and the one that is created are in fact not the same).
Did you have problems with this code?

Do we want/need to log something here?

Let's adjust the error here, it's not longer the repo that already exists but a project.

Should we log?

Considering the amount of error we're getting about too many files open when doing this operation, I'm considering shelling it out :(
Can do in another PR though

Because this avoids the test leakage in the tests:
The tests were working because this code got imported in a previous test, while later tests depended on it.

Shouldn't this be checking first if the project/fork is private? (or is it already doing it and I've missed it?)

", ".join(incompatible_hooks) ?

No, this is the case where e.g. TICKETS_FOLDER is disabled, in which case we don't have a repo to update as expected.
This case only is applicable when a feature is disabled by configuration.

Can this function be called multiple times over a single project? If so, won't this step fail then?

I think this will fail because one of the %s isn't filled

Yep, added.

Oh, could we move this to its own template? (cf settings_milestones.html for example), I'd like to reduce a bit the content of this template.

It could, if the migration failed. At which point, the admin (this is only available to admins) would need to check why it failed and manually clean things up.
I thought it was fair to assume they would clean this up, but if you disagree, I can make it automatically delete it if it exists already.

Should we print something else if the user isn't a pagure_admin? (Maybe just that only instance-wide admins can migrate project to repospanner).

Also, we may want to include a link somewhere about what repospanner is.

Fixed

Fixed.

Since this redirect mean something failed in the migration, I think we could redirect to the settings page (repospanner tab) rather than the front page, no?

Pretty sure you should have the original credit for this one ;-)

RE: Hm, this means we are no longer failing if we create a project with a git repo already existing on disk.

No, we still fail if a project already exists.
Just when running the task, and trying to create the repos, rather than before submitting the task.

Fixed

Then we should see about fixing the tests, as this change makes an optional dependency (on python-jenkins) mandatory :(

Since it's something performed in the UI, I would not be surprised if an admin tries a couple of times before digging further into it, so I think we should either clean it up or raise an exception telling the admin what the issue is (and thus how to fix it).

Jenkins and I are happy, let's merge :)