I have not look deep into the code itself, but the commit messages need to be reworked and it's missing tests :)

there is an issue about the projects becoming readonly when I set acl with the API.

Do you have the workers running?

yes, they are running.
some of the commit messages will vanish when I squash the commits. At least WIP is just to save my work for some reason

You're missing pagure.lib.git.generate_gitolite_acls(project=project)

This will take care of the read-only flag

that worked. Thanks !
Next step are the tests

I've added tests, squashed the commits and rebased onto upstream/master

I think it would be simpler to use a form here, it'll do all the validation for you

This wait argument isn't used anywhere, is it?

Where do you check if the user updating the ACL is allowed to do so?

working on those issues atm.

I've fixed the permission check and the unused variable. I've also added a test for the permission check.
I'm not sure if using a form for the input really helps here, it looks like I'd have to check the user and group anyway and doing the form for the ACL only isn't really worth the effort imho. Unless you have a strong opinion about this, I'd like to keep the current state.

I'm not sure if using a form for the input really helps here, it looks like I'd have to check the user and group anyway and doing the form for the ACL only isn't really worth the effort imho. Unless you have a strong opinion about this, I'd like to keep the current state.

One thing the form allows is ensuring something is provided when mandatory. It also means arguments are passed as HTTP POST argument rather than via the URL which makes it consistent with the rest of the API.
So yes, I think using a form here may be a good idea.

The current diff looks odd, as you the new endpoint wasn't created. Do you see the same thing locally?

Looks like it didn't pass CI: https://ci.centos.org/job/pagure-pr/623/

Oh finally :) Let me review this.

I think you should check that only one is supplied. Or add both ACLs.

indent

You should be able to simplify this, I think form.user.data.lower() or None should be sufficient (I'm pretty sure the .strip() is done automatically, feel free to double-check though)

I wonder if we even want the .lower(), I'd say: garbage-in = garbage-out, if you mistype the user name or group name, that's your problem.

If you're talking about checking if both, user and group are set, that's being checked in the forms validator.

I've removed .strip().lower() from user and group checking, but left .lower() for the acl

Pretty please pagure-ci rebuild

@karsten the re-triggering of the tests on PR change has been fixed in 4.0.4, so manual re-triggering should be less needed now :)

granted in this case a manual trigger was needed :)

You can do easier than with by using an SelectField instead of a TextField (it'll allow you to drop the entire block of validate_acl.

I'd even say "must be specified" instead of "need be used"

If we want to be fancy, we could do the opposite of the RequiredIf validator which is in pagure/hooks/__init__.py, an "InvalidIf" validator then ensures only one of the two fields is filled.

Let's also catch DB errors and rollback here :)

indentation?

I think we can use "elif" here, small optimization :)

Let's do this final tweak, squash, rebase and get it merged :)

(if jenkins passes)

fix those minor issues and also some flake8 warnings. jenkins #642 passed, but there will be another run with no code changes due to git squash

Commit be01e848 fixes this pull-request

hohoho, @pingou when can one expect this in prod? This would help me a lot ;)

@ignatenkobrain When we make 5.0 ready. :)