Packit needs pull_request_update and pull_request_rebase ACLs (available for the API/user token) to be able to update PRs it has already created. We would like to implement this feature: https://github.com/packit/packit/issues/2182
pull_request_update
pull_request_rebase
I already tried to work on it without success: https://pagure.io/pagure/pull-request/5436 https://pagure.io/fedora-infra/ansible/pull-request/1796
with the help of @zlopez (thank you!): element thread
It would be helpful having a suggestion on what needs to be changed in order for it to work.
We don't have a deadline.
@ngompa Do you know what we are missing?
Metadata Update from @zlopez: - Issue tagged with: Needs investigation
I would guess that the patch needs to be backported now? It is in mainline pagure coming as part of 6.0...
@ngompa According to https://src.fedoraproject.org/rpms/pagure/tree/epel8 it should be already backported as patch and the version running on pkgs01 is already on latest version.
Do we override CROSS_PROJECT_ACLS in the deployed pagure.cfg? If we do, then we need to add the ACL to that list.
CROSS_PROJECT_ACLS
pagure.cfg
We deployed it only on staging for now and we updated both CROSS_PROJECT_ACLS and USER_ACLS (here is the current config), but it's not available in web UI. I even checked the config on server itself and the values are really there.
USER_ACLS
Does the API work though?
It's not in the web UI because it wasn't added to the list enumerated in pagure/api/__init__.py and no written description.
pagure/api/__init__.py
Compare that change to this one (written by @zlopez), where there are updates to make it show up in the web UI: https://pagure.io/pagure/pull-request/5416
@ngompa This means that the patch needs to be updated as it only adds it to pagure/default_config.py.
pagure/default_config.py
Thanks for the help!
@mmassari Could you look into that?
Yes, sure. And thanks to both of you for the hints!
I gave a look at the code you linked to me and I am not sure of understanding what you are suggesting.
If I got it correctly @zlopez in this PR https://pagure.io/pagure/pull-request/5416 has created new API methods and then referenced them in pagure/api/__init__.py. I don't have created new API methods, they already existed and I think they are already referenced in pagure/api/__init__.py: https://github.com/Pagure/pagure/blob/c15e7251f70b2280e272698183c10554da02d3aa/pagure/api/__init__.py#L592-L593
But maybe I am misunderstanding your suggestion.
To show up in the web UI, they need descriptions to be processed for the generated documentation.
If it is the description missing, can it be that I need to create a migration like this one?
https://github.com/Pagure/pagure/blob/c15e7251f70b2280e272698183c10554da02d3aa/alembic/versions/21292448a775_update_acls_descriptions.py
That's if it's a permission that you can generate a token for, yes.
Metadata Update from @phsmoura: - Issue priority set to: Waiting on Assignee (was: Needs Review) - Issue tagged with: high-gain, high-trouble, ops
Yes, I need it for a token. I will try to create the migration. Thank you for the help.
Hi again :)
I was not totally sure that the migration (one similar to the one I linked above) would have solved my problems because the strings I see in the Web UI don't match the migration completely (as I would have expected). For this reason I decided to start a local Pagure instance to see if I could discover something more (sorry I did not do that before but I thought it would have been a simple change). Now I have this local Pagure instance that I start with podman-compose (I am following what I found in pagure/dev with a couple of small changes because I had a minor problem with a jinja template) and I can see that my latest patch (the one that adds the ACLs in the pagure.cfg file) makes the trick. I don't need anything else, I see the ACLs popup in web ui when I add them to the config file and they disappear when I remove them from the config file. For this reason I think I am missing something on the ansible side but I don't have any idea about what it could be. When I remove/add the ACLs from the pagure.cfg file I do podman-compose down and podman-compose up and this is taking down/up more than just apache. I don't know if this explains why I see it working locally and we can't see it working on stg. I also checked the database and I can confirm that the description for pull_request_[update|rebase] are already present in the acls table, at least in my local instance. Maybe you have new suggestions for me?
podman-compose down
podman-compose up
pull_request_[update|rebase]
acls
Thanks again!
The dev version of Pagure has plenty of changes that the deployed version doesn't have. So it's possible that the change you need is already in dev version, but it's not in the deployed version in Fedora infrastructure. You should try the local setup with the version that is deployed in Fedora Infrastructure, so you are on the same page.
Indeed this time it has worked!!! Checking out version 5.13.3 I can reproduce the problem. And descriptions are not there (in the database).
Now I am quite sure that what I am missing are the descriptions in the acls table.
But now my question is, should I create a downstream patch for release 5.13.3? With alambic pointing to the latest migration there? I don't think I have to commit anything in the Pagure repo, since the latest version of the Pagure code is already working.
But in this way will migrations diverge... What would you suggest?
We have a 5.13.x branch upstream for this stuff: https://pagure.io/pagure/commits/5.13.x
5.13.x
It would be nice if a pull request was made to collect up the backports and submit it there.
Ok, I will work on it tomorrow.
If the database migration for the table is "the same", it should be okay, then master just needs to be reconciled.
Hi, I had a brief look at the Pagure code base with git grep, it appears that the changed config figures in the following services:
git grep
pagure_ev
pagure_api_key_expire_mail
pagure_authorized_keys_worker
pagure_ci
pagure_docs_web
pagure_gitolite_worker
pagure_loadjson
pagure_logcom
pagure_mirror
pagure_mirror_project_in
pagure_web
pagure_webhook
pagure_worker
And they are started here (those marked with *): https://pagure.io/fedora-infra/ansible/blob/main/f/roles/distgit/pagure/tasks/main.yml#_340-357
*
I think we could factor those services out to some variable, so that it can be both start&enabled and restarted by the handler if needed.
@mfocko Thanks for looking at it, now I really think that the database descriptions are the problem.
@ngompa and @zlopez I was afraid of the idea of creating a divergent migration (I saw you don't have other divergents migrations in branch 5.13.x). And looking for ideas in the code I have found out that there exists a CLI command for Pagure that is able to update the acls table with the content of the Pagure configuration file. I can reproduce this workflow locally:
I add my ACL to the pagure.cfg and I run Pagure 5.13.x -> no ACLs are shown in Web UI I run python pagure/cli/admin.py --debug --config /code/dev/openshift.cfg update-acls -> ACLs appear in Web UI
python pagure/cli/admin.py --debug --config /code/dev/openshift.cfg update-acls
If you agree to not create a migration but just to run this command manually, I think I only need to create a PR in the ansible project adding my acl descriptions to the ACLS list.
That should be perfect. :100:
I created https://pagure.io/fedora-infra/ansible/pull-request/1825
I wait for someone to: 1. merge the PR 2. run ansible playbook 3. run the python pagure/cli/admin.py update-acls command
python pagure/cli/admin.py update-acls
If you let me know when the above is done, then I can check if it worked 🤞🏻
Thanks!
ok, just pushed this to prod. Please test and let us know if it's working or not. ;)
It's working 🎊 Thanks a lot to all for the support!
Awesome. Thanks for driving this through...
Metadata Update from @kevin: - Issue close_status updated to: Fixed with Explanation - Issue status updated to: Closed (was: Open)
Log in to comment on this ticket.