#2323 Add an API endpoint to modify a project's ownership
Merged by pingou. Opened by mprahl.
Unknown source master

Download 2323.patch

This PR adds a token ACL called "modify_project", which can be used to modify the ownership of a project on a new API endpoint.

I had trouble running tests that required celery, so I couldn't test the entire test suite, but the new tests I added all pass.

2 new commits added

  • Add an API to modify a Pagure project's owner
  • Move the logic of setting the project owner to another function

I've seen people not liking the terminology around owner, maybe we should reconsider it.
I don't have a one-word suggestion though :/

principle_maintainer or main_admin are what comes to my mind but :/

2 new commits added

  • Add an API to modify a Pagure project's owner
  • Move the logic of setting the project owner to another function

@pingou, since owner is used elsewhere in the API, let's keep it as is. For the Pagure-over-dist-git deployment, perhaps we can have a patch on the UI to change the terminology.

I think owner is only used in the settings page, so that's an easy one to fix.

I'm bringing this question now because I consider it still possible to change, the longer we wait the harder it will be to do.

@pingou I'd prefer not to change it in the API, but if you feel strongly about it, I can change it here and create a PR that changes the terminology in other use cases in the API. With that said, I have had trouble running the unit tests in Vagrant on the project as a whole since the rearchitecture. That would make it rather difficult to verify the renames.

2 new commits added

  • Add an API to modify a Pagure project's owner
  • Move the logic of setting the project owner to another function

@pingou I realized that I should be using the PATCH instead of PUT here, so I made that change.

To replace owner how about any of: steward , guardian , coordinator ?

If it's like "give project" from UI, then the condition should be : https://pagure.io/pagure/blob/master/f/pagure/ui/repo.py#_2584

Otherwise, the admins will be able to change the "main admin" via API and not via UI :p

So I want admins to be able to use this API, just not change the ownership.

You can see that I further restrict it below when the user tries to change the ownership:

if flask.g.fas_user != project.user:
    raise pagure.exceptions.APIError(401, error_code=APIERROR.ENOTOWNER)

oooo... so, this endpoint is not just about "giving project".

For now it can only be used to give a project, but I wrote it in a way so that we can expand it later on.

2 new commits added

  • Add an API to modify a Pagure project's owner
  • Move the logic of setting the project owner to another function

github has terms "owner" which can be multiple persons who can modify settings of projects and "collaborators" who can only have access to pushing code... So I would prefer to support multiple owners (and call them "admin"s) and everything else stays same.... Just my 5 cents about naming.

OK, :+1: to changing the language in the API but... can we make that a part of a separate change?

  • It will produce a backwards-incompatible API since owner everywhere will change to something_else.
  • This PR is just about being able to change that value, which is relatively simple compared to refactoring the whole API.

As for putting pagure over dist-git and concerns in the Fedora community about language, @mprahl and I can (try to) make sure that the client tooling calls this "point of contact" wherever users see it, even if under the hood the API calls call it "owner".

I filed a separate #2327 to take that up longer-term.

Is the language of the owner field the only item currently in debate for this PR?

In IRC in #pagure, @pingou suggested changing owner only in this new write API.. but keeping it in all of the other existing read APIs.

rebased

2 new commits added

  • Add an API to modify a Pagure project's owner
  • Move the logic of setting the project owner to another function

@pingou: I renamed owner to main_admin in this API. Please let me know if that works for you.

Can't we rely on tests.create_tokens ?

No try/except on sqlalchemyerror?

Couple of comments otherwise looks good, thanks! :)

rebased

@pingou, thanks for the review. This should be ready for another round!

rebased

I'm getting a couple of test failures, I'll fix them then we can merge :)

rebased

Pull-Request has been merged by pingou

Metadata