add readme :)

This is Jenkins token to verify it is coming from a legit source

So one more field in the db and one more field in the form for jenkins token needs to be there

repo?? this I guess is project.user.username

this will be session.commit

Only thing is jenkins token apart from that minor issues are there !

And that can be used to ensure only us are triggering the build on jenkins. Any reasons these two tokens should be different?

:thumbsup: for me !

Why does this run as git? I don't think this needs any access to files, right? Then we should be able to get a new user for privilege separation.

We probably should remove this?

Should this be configurable?

Coordinate this PR with the private repo one.

Why do all this messing around, and not just posixpath.join(base, project.path)?

Is this really the url that we need to provide? How about external PRs, and shouldn't this be a full clone URL?

Why a global variable that's shared?
Perhaps it would be better to have this be an internal thing, and have a getter to get access to this info.

The fact that this exists sounds wrong to me.
You should not be able to find a project by just the token, as that decreases the requirement from two to a single factor.
Instead, just get the project by the project name, and then validate the ci token in constant time.

This all assumes that this project is public. Maybe tell people to create a new user for Pagure CI, and use that here so that the Jenkins stuff doesn't need to be public? (see: private repo branch)

s/No PR found corresponding/No corresponding PR found/

s/tiven/given/

This should not need to be called outside of pagure.lib since it's porivate.
Missing some sort of wrapping function?

private*

it runs as git to avoid having to create a new user just for this, I don't know if using nobody would work

for this file, yes

:thumbsup:

it actually is, I should take these from the config iirc

If it's not needed, a new user would be preferred, since this one gives it access to more data then it needs.

agreed, but in a different PR if possible as we could then adjust other services while at it

Fixed using urlparse.urljoin

I do not know how that will work w/ remote PR for the moment, we'll need to test this

But that's unrelated to the URL provided here which is the URL of the project on jenkins

Same approach as REDIS above, it allows us to set it once when the app is started and re-use it then later in the code

w/o having to specify it to each and every function that could potentially use it

As long as the private repo changes aren't in master this is something for the person working on private repo :)

fixed

fixed

I'm fine with making it a public method but that's imho outside the scope of this PR

I'm not sure what this would gain us, could you expand?

Sure, but how about a get_pagure_ci() function, which would then be the only function accessing the global variable?

What's the benefit?

That it would become significantly harder to find a working token, since you need a correct project-token combination rather than just a token.

No global variables used across the code, making it easier to swap it out with some other implementation at a later stage.

Okay.

ack

hm ok, let me adjust the URL

Tbh, I would think that if an user activates CI on a private project, it would most likely be by their own will.

So either the jenkins instance is private as well, in which case no problem, of the jenkins instance is public and then there is not much we can do

How about a jenkins instance that has both public and private projects?

:thumbsup:

Many thanks for the review :)

fedmsg is no more needed for PagureCI I feel