Blockerbot for Forge #299

Manually merged
jgroman merged 1 commit from fix/296-blocker-review-voting-on-forge into develop 2026-04-29 14:33:53 +00:00
Owner

Implements blockerbugs app exension adding Forge support

Implements blockerbugs app exension adding Forge support
jgroman self-assigned this 2026-03-10 16:20:47 +00:00
jgroman requested review from kparal 2026-03-10 16:22:49 +00:00
jgroman force-pushed fix/296-blocker-review-voting-on-forge from ecc834176b to a1bd0f73be 2026-03-11 08:28:19 +00:00 Compare
We can discuss them separately afterwards. But they make the current PR much
harder to read.
kparal self-assigned this 2026-03-12 14:16:24 +00:00
kparal force-pushed fix/296-blocker-review-voting-on-forge from 5feb437643 to 9cad52a4a2 2026-03-12 14:26:46 +00:00 Compare
Owner

OK, my head officially exploded now. This is a massive patch. I decided to revert some unrelated changes, like formatting and other stuff, so that I have a chance to read the diff. I saved the previous branch locally and we can introduce those changes afterwards, when this massive PR is merged.

I also pushed 603872a5e6 to develop and rebased this PR, because those deps caused me lots of headaches.

I'll continue adding comments to individual lines now.

OK, my head officially exploded now. This is a massive patch. I decided to revert some unrelated changes, like formatting and other stuff, so that I have a chance to read the diff. I saved the previous branch locally and we can introduce those changes afterwards, when this massive PR is merged. I also pushed 603872a5e6ea33b5b354e91e91860155cdf72eda to `develop` and rebased this PR, because those deps caused me lots of headaches. I'll continue adding comments to individual lines now.
Owner

I decided to keep the generated method docstrings in the PR, because they help with understanding. But I'm unable to check their correctness, definitely not all of them, so let's hope there's nothing too misleading in there.

I decided to keep the generated method docstrings in the PR, because they help with understanding. But I'm unable to check their correctness, definitely not all of them, so let's hope there's nothing too misleading in there.
Owner

So I hacked things up to run this through ai-code-review, and here's the result - it has a few suggestions that look superficially sensible, see what you think.

So I hacked things up to run this through ai-code-review, and [here's the result](https://forge.stg.fedoraproject.org/quality/bbtest/pulls/1#issuecomment-184971) - it has a few suggestions that look superficially sensible, see what you think.
@ -77,0 +66,4 @@
FORGEJO_URL = "https://forge.stg.fedoraproject.org/"
FORGEJO_API = "https://forge.stg.fedoraproject.org/api/v1/"
FORGEJO_REPO = "quality/blocker-review"
FORGEJO_REPO_TOKEN = "YOUR SECRET FORGEJO API TOKEN"
Owner

Unlike for Pagure, in Forge, we don't have repo-specific tokens, but just user tokens, that have access to all repos the user can access, without limitation. Is that correct? In that case maybe we could rename this to FORGEJO_TOKEN or even FORGEJO_BOT_TOKEN to make this clearer?

Also, in a comment we could specify what permissions the token needs. Is it just write:issue or something more?

Unlike for Pagure, in Forge, we don't have repo-specific tokens, but just user tokens, that have access to all repos the user can access, without limitation. Is that correct? In that case maybe we could rename this to `FORGEJO_TOKEN` or even `FORGEJO_BOT_TOKEN` to make this clearer? Also, in a comment we could specify what permissions the token needs. Is it just `write:issue` or something more?
Owner

My note about adding a comment makes more sense to do in setting.py.example, I guess.

My note about adding a comment makes more sense to do in `setting.py.example`, I guess.
jgroman marked this conversation as resolved
@ -77,0 +67,4 @@
FORGEJO_API = "https://forge.stg.fedoraproject.org/api/v1/"
FORGEJO_REPO = "quality/blocker-review"
FORGEJO_REPO_TOKEN = "YOUR SECRET FORGEJO API TOKEN"
FORGEJO_REPO_WEBHOOK_SECRET = "YOUR WEBHOOK SECRET"
Owner

Maybe we should say in a comment that this webhook should be triggered on all issue events, here and/or in some documentation.

Maybe we should say in a comment that this webhook should be triggered on all issue events, here and/or in some documentation.
Owner

My note about adding a comment makes more sense to do in setting.py.example.

My note about adding a comment makes more sense to do in `setting.py.example`.
jgroman marked this conversation as resolved
@ -157,0 +298,4 @@
state = data.get("issue", {}).get("state", "")
if not issue_number or not state:
msg = "Unable to parse received message (issue number, state)"
Owner

I don't see data being logged anywhere (or least both issue_number and state), so it might be hard to reconstruct what happened here, if we don't print out the original message?

I don't see `data` being logged anywhere (or least both `issue_number` and `state`), so it might be hard to reconstruct what happened here, if we don't print out the original message?
jgroman marked this conversation as resolved
@ -8,2 +10,4 @@
from blockerbugs.models.bug import Bug
# URL path component that identifies Forgejo issue links
FORGEJO_ISSUES_PATH = "/issues/"
Owner

This is interesting. Do I understand correctly that you need to distinguish discussions that link to Pagure from discussions that link to Forge (so that when mass-closing them, you don't use Forge API calls to Pagure URLs), but you don't want to distinguish it based on hostname? In other words, is this here just because of the Pagure -> Forge transition, but that's the only purpose? If that is so, it might deserve a more explanatory comment.

What if we instead compared the stored discussion URL with FORGEJO_URL + FORGEJO_REPO from settings, and made it a generally useful check? It would make sure it doesn't touch tickets from other repos than the specific one configured in project settings.

This is interesting. Do I understand correctly that you need to distinguish discussions that link to Pagure from discussions that link to Forge (so that when mass-closing them, you don't use Forge API calls to Pagure URLs), but you don't want to distinguish it based on hostname? In other words, is this here just because of the Pagure -> Forge transition, but that's the only purpose? If that is so, it might deserve a more explanatory comment. What if we instead compared the stored discussion URL with `FORGEJO_URL + FORGEJO_REPO` from settings, and made it a generally useful check? It would make sure it doesn't touch tickets from other repos than the specific one configured in project settings.
jgroman marked this conversation as resolved
@ -25,3 +53,2 @@
def create_discussions_links(milestone, bugs, links_to_reuse={}):
app.logger.info('Creating discussion tickets for %d bugs under %s' % (
def create_forgejo_discussions_links(
Owner

These function names received "forgejo" in their name when there were both pagure and forgejo implementations here. Now that we dropped Pagure, let's go back to their original names? So create_discussions_links(). It will make it shorter and the diff simpler.

These function names received "forgejo" in their name when there were both pagure and forgejo implementations here. Now that we dropped Pagure, let's go back to their original names? So `create_discussions_links()`. It will make it shorter and the diff simpler.
jgroman marked this conversation as resolved
@ -52,3 +106,2 @@
def sync_discussions():
if app.config["PAGURE_REPO_TOKEN"] in (bb_Config.PAGURE_REPO_TOKEN, ""):
def sync_forgejo_discussions() -> None:
Owner

This can be just sync_discussions().

This can be just `sync_discussions()`.
jgroman marked this conversation as resolved
@ -84,3 +176,1 @@
not yet marked with ``Release.discussions_closed=True``.
'''
app.logger.info('Closing discussion tickets in inactive releases...')
def close_forgejo_discussions(dry_run: bool = False) -> None:
Owner

In this case, the "inactive releases" in name seemed pretty significant to me, because you see it right from the function name. I would keep original close_discussions_inactive_releases() name.

In this case, the "inactive releases" in name seemed pretty significant to me, because you see it right from the function name. I would keep original `close_discussions_inactive_releases()` name.
jgroman marked this conversation as resolved
@ -77,0 +72,4 @@
FORGEJO_BOT_ENABLED = True
FORGEJO_BOT_LOOP_THRESHOLD = 2
FORGEJO_ADMIN_ORG = "quality"
FORGEJO_ADMIN_TEAM = "reviewers"
Owner

In production we have the members team that probably should have this rights. So let's use "members" here, and create the same group on stg, so that we again have it synchronized and don't need to change it.

These two options should also be added to settings.py.example, I think.

In production we have the [members](https://forge.fedoraproject.org/org/quality/teams/members) team that probably should have this rights. So let's use `"members"` here, and create the same group on stg, so that we again have it synchronized and don't need to change it. These two options should also be added to `settings.py.example`, I think.
Owner

Hmm, I'm struggling with this a bit. In production, we have two teams - Owners (who have admin access to the org) and members (who have admin access to all repos). In staging, we only have Owners. I created members, but I have no way to populate it - it requires a matching FAS group, it can't be populated manually. So in order to test this, I think we need:

a) ask Infra to create a staging setup that is the same as production setup, i.e. also map members to the right FAS group (on stg).

In production, I see these FAS groups:

forge-quality-members
Members team for the Quality organization on Fedora Forge. Membership to this team is inherited via qa-tools-sig

forge-quality-owners
Owners team for the quality organization on Fedora Forge. Membership to this team is inherited via the qa-admin group

b) In our staging config, use FORGEJO_ADMIN_TEAM = "Owners" and hope that it will work fine once we deploy it to production


Furthermore (a separate issue), I saw that blockerbot was part of Owners on staging. We don't want this, that's a security issue. I dropped it from the respective FAS groups, logged in with the account, and the group was updated, it's no longer an owner. Instead, I added blockerbot as a collaborator with write access - that should be sufficient for it to be able to update tickets. We need to test this, though.

//Edit: Actually, maybe it doesn't need to be even a collaborator, maybe just the API token access is sufficient? We should try both.

//Edit2: No, that wouldn't make sense. The token is a user token, configured in user settings. The token allows to operate on objects that the user has access to. So access to a particular repo tickets must be granted. I'll document this.

Hmm, I'm struggling with this a bit. In [production](https://forge.fedoraproject.org/org/quality/teams), we have two teams - _Owners_ (who have admin access to the org) and _members_ (who have admin access to all repos). In [staging](https://forge.stg.fedoraproject.org/org/quality/teams), we only have _Owners_. I created _members_, but I have no way to populate it - it requires a matching FAS group, it can't be populated manually. So in order to test this, I think we need: a) ask Infra to create a staging setup that is the same as production setup, i.e. also map members to the right FAS group (on stg). In production, I see these FAS groups: > forge-quality-members > Members team for the Quality organization on Fedora Forge. Membership to this team is inherited via `qa-tools-sig` > > forge-quality-owners > Owners team for the quality organization on Fedora Forge. Membership to this team is inherited via the `qa-admin` group b) In our staging config, use `FORGEJO_ADMIN_TEAM = "Owners"` and hope that it will work fine once we deploy it to production ---- Furthermore (a separate issue), I saw that [blockerbot](https://forge.stg.fedoraproject.org/blockerbot) was part of _Owners_ on staging. We don't want this, that's a security issue. I dropped it from the respective FAS groups, logged in with the account, and the group was updated, it's no longer an owner. Instead, I added blockerbot as a [collaborator with write access](https://forge.stg.fedoraproject.org/quality/blocker-review/settings/collaboration) - that should be sufficient for it to be able to update tickets. We need to test this, though. //Edit: Actually, maybe it doesn't need to be even a collaborator, maybe just the API token access is sufficient? We should try both. //Edit2: No, that wouldn't make sense. The token is a _user token_, configured in user settings. The token allows to operate on objects that the user has access to. So access to a particular repo tickets must be granted. I'll document this.
Owner

a) ask Infra to create a staging setup that is the same as production setup, i.e. also map members to the right FAS group (on stg).

I've created a ticket here:
infra/tickets#13232

> a) ask Infra to create a staging setup that is the same as production setup, i.e. also map members to the right FAS group (on stg). I've created a ticket here: https://forge.fedoraproject.org/infra/tickets/issues/13232
kparal marked this conversation as resolved
@ -135,1 +136,3 @@
"PAGURE_REPO", "PAGURE_BOT_USERNAME", "PAGURE_BOT_ENABLED", "PAGURE_URL", "PAGURE_API",
additional_env_keys = ["FAS_ADMIN_GROUP", "FORGEJO_REPO_TOKEN", "FORGEJO_REPO_WEBHOOK_SECRET", "FORGEJO_REPO",
"FORGEJO_BOT_USERNAME", "FORGEJO_BOT_ENABLED", "FORGEJO_URL", "FORGEJO_API",
"FORGEJO_ADMIN_ORG", "FORGEJO_ADMIN_TEAM", "DISCUSSION_SYSTEM",
Owner

I think DISCUSSION_SYSTEM is a leftover from previous code and can be deleted. It's also present in some other files, not just here.

I think `DISCUSSION_SYSTEM` is a leftover from previous code and can be deleted. It's also present in some other files, not just here.
jgroman marked this conversation as resolved
@ -21,1 +16,3 @@
PAGURE_BOT_USERNAME = 'blockerbot'
FORGEJO_URL = "https://forge.stg.fedoraproject.org/"
FORGEJO_API = "https://forge.stg.fedoraproject.org/api/v1/"
FORGEJO_REPO = "quality/blocker-review-test"
Owner

I renamed the repo to quality/blocker-review, but forgot to change it here as well. Oh well, let's fix it.

I renamed the repo to` quality/blocker-review`, but forgot to change it here as well. Oh well, let's fix it.
jgroman marked this conversation as resolved
@ -0,0 +1,97 @@
# BlockerBugs Application - High-Level Workflow
Owner

Is this a leftover that you created while trying to understand the code, and forgot to delete? Or do you think we should keep this in? I'm personally unsure. It contains a lot of really specific details which we go stale in time. And using AI people can generate a fresh similar document locally any time. It also contains some invalid info already (e.g. there's no flask sync-bugs command, that's not how it's executed), so I'd need to go through it a check it properly.

So there is some usefulness, but I'd probably go and rewrite it manually, strip details, and make it valid for a longer time.

Is this a leftover that you created while trying to understand the code, and forgot to delete? Or do you think we should keep this in? I'm personally unsure. It contains a lot of really specific details which we go stale in time. And using AI people can generate a fresh similar document locally any time. It also contains some invalid info already (e.g. there's no `flask sync-bugs` command, that's not how it's executed), so I'd need to go through it a check it properly. So there is some usefulness, but I'd probably go and rewrite it manually, strip details, and make it valid for a longer time.
jgroman marked this conversation as resolved
@ -36,2 +35,3 @@
--env POSTGRESQL_DATABASE=blockerbugs \
--publish 5432:5432 \
docker.io/library/postgres:13
quay.io/fedora/postgresql-13:latest
Owner

What is the reason for switching this to the quay.io image? (I'm not saying it's wrong, I simply don't know anything about it).

What is the reason for switching this to the quay.io image? (I'm not saying it's wrong, I simply don't know anything about it).
Author
Owner

Using images from Docker Hub is rate limited and slower than using our own infrastructure.

Using images from Docker Hub is rate limited and slower than using our own infrastructure.
jgroman marked this conversation as resolved
@ -43,3 +44,2 @@
$ podman exec -it blockerbugsdb psql --host=localhost --username=postgres --command='select version();'
Password for user postgres:
$ podman exec -it blockerbugsdb psql --host=localhost --username=fedora --command='select version();'
Owner

I can't make this work. If I create the container as documented above, then:

$ podman exec -it blockerbugsdb psql --host=localhost --username=fedora --command='select version();'
psql: error: FATAL:  database "fedora" does not exist

The original instructions (with the image pulled from docker.io) work fine.

I can't make this work. If I create the container as documented above, then: ``` $ podman exec -it blockerbugsdb psql --host=localhost --username=fedora --command='select version();' psql: error: FATAL: database "fedora" does not exist ``` The original instructions (with the image pulled from docker.io) work fine.
jgroman marked this conversation as resolved
@ -203,6 +204,136 @@ While the virtualenv is active, you can build all documentation with this comman
You can then inspect the resulting docs in ``build/docs/html``. Open ``index.html`` to see the home page. Not everything is built as HTML and linked from there, so check directory contents as well, or even the docs source directory at ``docs/source``.
Forgejo Integration
Owner

This seems too chatty for a general help, did you intend to keep this in, or remove it?

I think most (or maybe all) of it doesn't really fit into development.rst and instead should be partially moved to installation.rst (how to configure the app) and blockerbugscli.rst (the commands). Some other parts (the workflows and troubleshooting) could go to a new file, e.g. usage.md (with expectation that we will drop rst in the future and convert everything to md, I'm +1 to that). Thoughts?

This seems too chatty for a general help, did you intend to keep this in, or remove it? I think most (or maybe all) of it doesn't really fit into `development.rst` and instead should be partially moved to `installation.rst` (how to configure the app) and `blockerbugscli.rst` (the commands). Some other parts (the workflows and troubleshooting) could go to a new file, e.g. `usage.md` (with expectation that we will drop rst in the future and convert everything to md, I'm +1 to that). Thoughts?
jgroman marked this conversation as resolved
@ -15,3 +15,3 @@
then select a desired milestone and for each proposed blocker/freeze exception you'll see a **Vote** link displayed, which will direct you to the matching discussion ticket.
If a bug report is very complex and requires real-time communication instead of asynchronous ticket discussions, we'll tag the ticket with the [meeting tag](https://pagure.io/fedora-qa/blocker-review/issues?status=Open&tags=meeting) and the discussion will take place during a blocker bug meeting (see above) instead of here.
If a bug report is very complex and requires real-time communication instead of asynchronous ticket discussions, we'll tag the ticket with the [meeting tag](https://forge.fedoraproject.org/quality/blocker-review/issues?status=Open&tags=meeting) and the discussion will take place during a blocker bug meeting (see above) instead of here.
Owner

The new URL doesn't work - the repo doesn't exist, but if it did, Forge doesn't seem to be use to use tag names in HTTP params, only tag IDs. So we need to create the tag first, to be able provide correct URL. However, we don't really use the meeting tag at all, so... I'll just rewrite this paragraph and replace it with something that doesn't need that link.

The new URL doesn't work - the repo doesn't exist, but if it did, Forge doesn't seem to be use to use tag names in HTTP params, only tag IDs. So we need to create the tag first, to be able provide correct URL. However, we don't really use the meeting tag at all, so... I'll just rewrite this paragraph and replace it with something that doesn't need that link.
Owner

Adjusted

Adjusted
kparal marked this conversation as resolved
@ -72,3 +72,3 @@
### Administrative commands
Members of the [fedora-qa](https://pagure.io/group/fedora-qa) Pagure group can issue administrative commands. Those commands are handled in the same way as voting commands, i.e. they need to be placed on a separate line and are case-insensitive. Those commands include:
Members of the [fedora-qa](https://forge.fedoraproject.org/quality/) Forge organization can issue administrative commands. Those commands are handled in the same way as voting commands, i.e. they need to be placed on a separate line and are case-insensitive. Those commands include:
Owner

It's now named Quality, not fedora-qa. I'll adjust it.

It's now named Quality, not fedora-qa. I'll adjust it.
Owner

Adjusted

Adjusted
kparal marked this conversation as resolved
pytest.ini Outdated
@ -0,0 +1,23 @@
[pytest]
Owner

I wonder why you needed to moved it out of setup.cfg, does pytest no longer load it? It seems it still does, even though they have a warning there. I chose setup.cfg back in the days, because it was the only option to have all configuration in a single file, instead of having lots of individual files for all the tools. If it no longer works, we can look into replacing it with something modern.

Gemini claims that the current modern standard is pyproject.toml, which is supported by all those tools except flake8 🙁 It recommends to replace flake8 with ruff.

I see that you already created pyproject.toml, so if you had issues with pytest configuration in setup.cfg, why not move it directly to pyproject.toml?

I wonder why you needed to moved it out of `setup.cfg`, does `pytest` no longer load it? It seems it [still does](https://docs.pytest.org/en/stable/reference/customize.html#setup-cfg), even though they have a warning there. I chose `setup.cfg` back in the days, because it was the only option to have all configuration in a single file, instead of having lots of individual files for all the tools. If it no longer works, we can look into replacing it with something modern. Gemini claims that the current modern standard is `pyproject.toml`, which is supported by all those tools except `flake8` 🙁 It recommends to replace `flake8` with `ruff`. I see that you already created `pyproject.toml`, so if you had issues with pytest configuration in `setup.cfg`, why not move it directly to `pyproject.toml`?
Author
Owner

Having a pytest configuration in setup.cfg requires use of double % to properly escape pytest formatting strings for use in OpenShift environment. However using double % in format strings in setup.cfg breaks pytest in local environment. Current workaround is to move pytest config out of setup.cfg to either pytest.ini or pyproject.toml. I used pytest.ini to have pytest config visibly separated from anything else but I have no problem to use pyproject.toml as well. Will move the pytest config in there.

Having a pytest configuration in `setup.cfg` requires use of double % to properly escape pytest formatting strings for use in OpenShift environment. However using double % in format strings in `setup.cfg` breaks pytest in local environment. Current workaround is to move pytest config out of `setup.cfg` to either `pytest.ini` or `pyproject.toml`. I used `pytest.ini` to have pytest config visibly separated from anything else but I have no problem to use `pyproject.toml` as well. Will move the pytest config in there.
jgroman marked this conversation as resolved
@ -2,6 +2,7 @@
# this automatically also pulls rope and flake8 (which pulls pyflakes, pycodestyle and mccabe)
python-language-server[rope,flake8]
mypy
testcontainers ~= 4.14.0
Owner

This will be dependent on the outcome of #96, it's the same problem as for testdays-web. Actually a bigger one, because testdays-web doesn't really need it (the database can be sqlite), but we're running a whole forgejo in a container. More on that topic separately.

This will be dependent on the outcome of #96, it's the same problem as for testdays-web. Actually a bigger one, because testdays-web doesn't really need it (the database can be sqlite), but we're running a whole forgejo in a container. More on that topic separately.
Author
Owner

I think you meant quality/testdays-web#96 , right?
I think I have a workable solution for that one - see quality/testdays-web#99 - we can just create Forge action service containers with Postgres and Forgejo and use those when running in CI.

I think you meant https://forge.fedoraproject.org/quality/testdays-web/issues/96 , right? I think I have a workable solution for that one - see https://forge.fedoraproject.org/quality/testdays-web/pulls/99 - we can just create Forge action service containers with Postgres and Forgejo and use those when running in CI.
kparal marked this conversation as resolved
requirements.txt Outdated
@ -17,6 +17,7 @@ munch
psycopg[binary,pool]
py
pycurl
pyforgejo
Owner

Should we use >= or ~= here? I have two minds about it, I wonder what's your preferred approach.

For development, I think it useful to run on the latest stuff, to notice any breakage as soon as possible.

For production, it's good to pin versions most probably with ~=, because that prevents things from breaking randomly and at any time. (Our production can be re-deployed at any time, without our action, just because somebody needs to re-run playbooks for some reason).

The problem is that this same file is used for both development and production 😆 So until we have a better approach, I guess it's better to pin versions here as well, to make production stable.

WDYT?

Should we use `>=` or `~=` here? I have two minds about it, I wonder what's your preferred approach. For development, I think it useful to run on the latest stuff, to notice any breakage as soon as possible. For production, it's good to pin versions most probably with `~=`, because that prevents things from breaking randomly and at any time. (Our production can be re-deployed at any time, without our action, just because somebody needs to re-run playbooks for some reason). The problem is that this same file is used for both development and production 😆 So until we have a better approach, I guess it's better to pin versions here as well, to make production stable. WDYT?
Owner

FWIW, I don't ever like pinning versions (as a general policy) at all, because then you've just created a big problem for yourself: you need to come up with a policy and mechanism for updating the pins.

I prefer to just always use no version specification, or >= if appropriate, and address issues as they arise. If you have a good test suite and a staging deployment, you have plenty of opportunities to catch and address issues with new versions of dependencies before they reach production, and then you're not in danger of getting stuck on ancient versions and having to do a huge catch-up commit.

It's kinda the "Fedora vs RHEL" question, and we're Fedora, sooo...:D

FWIW, I don't ever like pinning versions (as a general policy) at all, because then you've just created a big problem for yourself: you need to come up with a policy and mechanism for updating the pins. I prefer to just always use no version specification, or `>=` if appropriate, and address issues as they arise. If you have a good test suite and a staging deployment, you have plenty of opportunities to catch and address issues with new versions of dependencies before they reach production, and then you're not in danger of getting stuck on ancient versions and having to do a huge catch-up commit. It's kinda the "Fedora vs RHEL" question, and we're Fedora, sooo...:D
Owner

For production, it's good to pin versions most probably with ~=, because that prevents things from breaking randomly and at any time. (Our production can be re-deployed at any time, without our action, just because somebody needs to re-run playbooks for some reason).

Re-running the playbooks doesn't have to include rebuilding the container image. But another way to address this would perhaps be to have versioned tags, or a 'production' branch, with pinned dependencies, and have the production deployment done from that tag/branch; develop could have unpinned dependencies?

> For production, it's good to pin versions most probably with ~=, because that prevents things from breaking randomly and at any time. (Our production can be re-deployed at any time, without our action, just because somebody needs to re-run playbooks for some reason). Re-running the playbooks doesn't have to include rebuilding the container image. But another way to address this would perhaps be to have versioned tags, or a 'production' branch, with pinned dependencies, and have the production deployment done from that tag/branch; `develop` could have unpinned dependencies?
Author
Owner

I'd also prefer to get rid of pins as much as possible, but I suppose at least production would deserve to have some.
We can have two requirements-[stg,prod].txt files and just feed the required name into PIP_REQUIREMENTS env var before starting deployment.
Another option apparently (according to Claude Code) is moving setup.py into pyproject.toml, defining dependency groups there and generating requirements.txt at runtime.
But I'd suggest opening a separate issue for such an update.

I'd also prefer to get rid of pins as much as possible, but I suppose at least production would deserve to have some. We can have two requirements-[stg,prod].txt files and just feed the required name into PIP_REQUIREMENTS env var before starting deployment. Another option apparently (according to Claude Code) is moving `setup.py` into `pyproject.toml`, defining dependency groups there and generating requirements.txt at runtime. But I'd suggest opening a separate issue for such an update.
Owner

Ok, let's deal with this some other time. Leaving pyforgejo unpinned is probably OK for the moment.

Ok, let's deal with this some other time. Leaving `pyforgejo` unpinned is probably OK for the moment.
kparal marked this conversation as resolved
jgroman changed title from WIP: Blockerbot for Forge to Blockerbot for Forge 2026-03-17 08:47:39 +00:00
Author
Owner

Ready for review

Ready for review
Owner

Can we do the same thing as in testdays-web to make this work with both testcontainers and CI-provided containers? Or would you prefer to merge this first then do that in #300 ?

I sort of think it might be nice to do the opposite - merge #300 first so we have CI and AI review, then this PR would have to pass those checks. WDYT?

Can we do the same thing as in testdays-web to make this work with both testcontainers and CI-provided containers? Or would you prefer to merge this first then do that in https://forge.fedoraproject.org/quality/blockerbugs/pulls/300 ? I sort of think it might be nice to do the opposite - merge #300 first so we have CI and AI review, then *this* PR would have to pass those checks. WDYT?
Author
Owner

Agreed, this is good idea, let's do #300 first. Do you want me to update conftest.py?

Agreed, this is good idea, let's do #300 first. Do you want me to update `conftest.py`?
Owner

Sorry, update it where? Here? #300 ? #300 should work correctly at present AFAIK. Then any adjustments to make the new test setup in this PR work with the CI setup from #300 should be done here.

Sorry, update it where? Here? #300 ? #300 should work correctly *at present* AFAIK. Then any adjustments to make the new test setup in this PR work with the CI setup from #300 should be done here.
Author
Owner

Right, OK. My bad, got it mixed up with testdays-web updates.
So to sum it up the plan is to merge #300 first and then do CI service containers update here in this PR. No problem with that!

Right, OK. My bad, got it mixed up with testdays-web updates. So to sum it up the plan is to merge #300 first and then do CI service containers update here in this PR. No problem with that!
Owner

OK. @kparal do you agree with the plan?

OK. @kparal do you agree with the plan?
adamwill force-pushed fix/296-blocker-review-voting-on-forge from 34ed8bf956 to 2c1d5a18d2
Some checks failed
Run tests and linters / test (pull_request) Failing after 2m31s
Run tests and linters / lint (pull_request) Has been skipped
AI Code Review / ai-review (pull_request_target) Successful in 35s
2026-03-25 16:02:19 +00:00
Compare

AI Code Review

📋 MR Summary

This MR implements support for the Forgejo platform in the blockerbugs app, replacing the previous Pagure integration. It introduces a new forgejo_interface and forgejo_bot, updates webhook endpoints, and adjusts related configurations and sync logic.

  • Key Changes:
    • Replaced Pagure integration (pagure_bot.py, pagure_interface.py) with Forgejo equivalents (forgejo_bot.py, forgejo_interface.py).
    • Updated configuration keys from PAGURE_* to FORGEJO_* and added team/org-based admin validation.
    • Added a new API endpoint /api/v0/webhook/forgejo for handling issue comments from Forgejo.
    • Introduced comprehensive test suites (unit, integration, e2e) using Testcontainers for PostgreSQL and Forgejo.
  • Impact: blockerbugs/cli.py, blockerbugs/config.py, blockerbugs/controllers/api/api.py, blockerbugs/controllers/api/utils.py, blockerbugs/util/discussion_sync.py, blockerbugs/util/forgejo_bot.py, blockerbugs/util/forgejo_interface.py, testing/*
  • Risk Level: 🟡 Medium - The changes touch core functionality (webhook handling, voting logic, issue synchronization) and replace a major external integration. While the test coverage is extensive, the transition from Pagure to Forgejo inherently carries some risk.

Detailed Code Review

The implementation is comprehensive and well-structured, successfully migrating the voting and webhook logic from Pagure to Forgejo. The transition leverages standard pyforgejo methods and introduces testcontainers for more robust testing.

The logic in the webhook and the API handlers appears solid, effectively parsing commands, checking HMAC signatures, and interacting with the database. The security aspect of verifying webhooks and validating users via the Forgejo API is well-handled.

No critical bugs or security vulnerabilities were identified in the core logic. Minor suggestions are mostly related to ensuring edge cases are fully handled.

Summary

  • Overall Assessment: No critical issues identified. The transition to Forgejo is well-executed with comprehensive tests. Minor suggestions provided for testing coverage.

🤖 AI Code Review | Generated with ai-code-review | Model: gemini-3.1-pro-preview

⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.

## AI Code Review ### 📋 MR Summary This MR implements support for the Forgejo platform in the blockerbugs app, replacing the previous Pagure integration. It introduces a new `forgejo_interface` and `forgejo_bot`, updates webhook endpoints, and adjusts related configurations and sync logic. - **Key Changes:** - Replaced Pagure integration (`pagure_bot.py`, `pagure_interface.py`) with Forgejo equivalents (`forgejo_bot.py`, `forgejo_interface.py`). - Updated configuration keys from `PAGURE_*` to `FORGEJO_*` and added team/org-based admin validation. - Added a new API endpoint `/api/v0/webhook/forgejo` for handling issue comments from Forgejo. - Introduced comprehensive test suites (unit, integration, e2e) using Testcontainers for PostgreSQL and Forgejo. - **Impact:** blockerbugs/cli.py, blockerbugs/config.py, blockerbugs/controllers/api/api.py, blockerbugs/controllers/api/utils.py, blockerbugs/util/discussion_sync.py, blockerbugs/util/forgejo_bot.py, blockerbugs/util/forgejo_interface.py, testing/* - **Risk Level:** 🟡 Medium - The changes touch core functionality (webhook handling, voting logic, issue synchronization) and replace a major external integration. While the test coverage is extensive, the transition from Pagure to Forgejo inherently carries some risk. ### Detailed Code Review The implementation is comprehensive and well-structured, successfully migrating the voting and webhook logic from Pagure to Forgejo. The transition leverages standard pyforgejo methods and introduces testcontainers for more robust testing. The logic in the webhook and the API handlers appears solid, effectively parsing commands, checking HMAC signatures, and interacting with the database. The security aspect of verifying webhooks and validating users via the Forgejo API is well-handled. No critical bugs or security vulnerabilities were identified in the core logic. Minor suggestions are mostly related to ensuring edge cases are fully handled. ### ✅ Summary - **Overall Assessment:** No critical issues identified. The transition to Forgejo is well-executed with comprehensive tests. Minor suggestions provided for testing coverage. --- 🤖 **AI Code Review** | Generated with [ai-code-review](https://gitlab.com/redhat/edge/ci-cd/ai-code-review) | **Model:** `gemini-3.1-pro-preview` ⚠️ *AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.*
Owner

pretty useless AI review! (note its positive assertions are usually not worth taking seriously; it's mainly useful when it notices a problem). Oh, well. the test failure looks significant.

pretty useless AI review! (note its positive assertions are usually not worth taking seriously; it's mainly useful when it notices a problem). Oh, well. the test failure looks significant.
Author
Owner

Yes, the whole container scaffolding in CI workflow and conftest.py needs to be implemented for tests to pass. I was waiting for #300 merge, will work on this now.

Yes, the whole container scaffolding in CI workflow and `conftest.py` needs to be implemented for tests to pass. I was waiting for #300 merge, will work on this now.
jgroman force-pushed fix/296-blocker-review-voting-on-forge from 2c1d5a18d2
Some checks failed
Run tests and linters / test (pull_request) Failing after 2m31s
Run tests and linters / lint (pull_request) Has been skipped
AI Code Review / ai-review (pull_request_target) Successful in 35s
to 81d0ed301e
Some checks failed
Run tests and linters / test (pull_request) Failing after 2m37s
Run tests and linters / lint (pull_request) Has been skipped
2026-03-26 10:40:47 +00:00
Compare
Update tests to work with CI containers
Some checks failed
Run tests and linters / test (pull_request) Failing after 2m45s
Run tests and linters / lint (pull_request) Has been skipped
d2fac18966
Fix problems talking to Forgejo in CI container
Some checks failed
Run tests and linters / test (pull_request) Successful in 2m44s
Run tests and linters / lint (pull_request) Failing after 1s
AI Code Review / ai-review (pull_request_target) Successful in 44s
4a10f4a307

AI Code Review

📋 MR Summary

This merge request replaces Pagure integration with Forgejo for blocker bug review discussions and voting.

  • Key Changes:
    • Replaced all Pagure-related configurations, bots, and API interactions with Forgejo equivalents.
    • Added a new ForgejoInterface wrapper utilizing the pyforgejo library.
    • Updated the webhook endpoint and payload validation to process Forgejo's issue_comment events via HMAC-SHA256.
    • Replaced SQLite database in development with PostgreSQL to ensure consistency with production.
    • Introduced comprehensive end-to-end and integration tests utilizing testcontainers for Forgejo and PostgreSQL.
  • Impact: blockerbugs/config.py, blockerbugs/cli.py, blockerbugs/controllers/api/api.py, blockerbugs/util/forgejo_bot.py, blockerbugs/util/forgejo_interface.py, blockerbugs/util/discussion_sync.py, testing/*
  • Risk Level: 🟡 Medium - The PR involves a complete replacement of the core communication and voting backbone of the blockerbugs application. Any overlooked differences in how Forgejo formats payloads, structures data, or handles rate limiting compared to Pagure could break the voting workflow.

Detailed Code Review

The migration to Forgejo appears well-thought-out and comprehensive. The implementation cleanly replaces the old Pagure code while maintaining the existing voting logic. The use of testcontainers for end-to-end testing of the Forgejo and Postgres integrations is a substantial improvement in the project's testing methodology. Overall, the logic appears sound and secure, properly validating Forgejo webhook signatures in a constant-time manner.

Summary

  • Overall Assessment: The changes look solid and well-tested. No critical logic or security issues were found.
  • Minor Suggestions:
    • Consider using request.get_data() instead of request.data in the Flask webhook handler as a safer way to access the raw payload for HMAC verification, though request.data works for JSON payloads.

🤖 AI Code Review | Generated with ai-code-review | Model: gemini-3.1-pro-preview

⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.

## AI Code Review ### 📋 MR Summary This merge request replaces Pagure integration with Forgejo for blocker bug review discussions and voting. - **Key Changes:** - Replaced all Pagure-related configurations, bots, and API interactions with Forgejo equivalents. - Added a new `ForgejoInterface` wrapper utilizing the `pyforgejo` library. - Updated the webhook endpoint and payload validation to process Forgejo's `issue_comment` events via HMAC-SHA256. - Replaced SQLite database in development with PostgreSQL to ensure consistency with production. - Introduced comprehensive end-to-end and integration tests utilizing `testcontainers` for Forgejo and PostgreSQL. - **Impact:** blockerbugs/config.py, blockerbugs/cli.py, blockerbugs/controllers/api/api.py, blockerbugs/util/forgejo_bot.py, blockerbugs/util/forgejo_interface.py, blockerbugs/util/discussion_sync.py, testing/* - **Risk Level:** 🟡 Medium - The PR involves a complete replacement of the core communication and voting backbone of the blockerbugs application. Any overlooked differences in how Forgejo formats payloads, structures data, or handles rate limiting compared to Pagure could break the voting workflow. ### Detailed Code Review The migration to Forgejo appears well-thought-out and comprehensive. The implementation cleanly replaces the old Pagure code while maintaining the existing voting logic. The use of testcontainers for end-to-end testing of the Forgejo and Postgres integrations is a substantial improvement in the project's testing methodology. Overall, the logic appears sound and secure, properly validating Forgejo webhook signatures in a constant-time manner. ### ✅ Summary - **Overall Assessment:** The changes look solid and well-tested. No critical logic or security issues were found. - **Minor Suggestions:** - Consider using `request.get_data()` instead of `request.data` in the Flask webhook handler as a safer way to access the raw payload for HMAC verification, though `request.data` works for JSON payloads. --- 🤖 **AI Code Review** | Generated with [ai-code-review](https://gitlab.com/redhat/edge/ci-cd/ai-code-review) | **Model:** `gemini-3.1-pro-preview` ⚠️ *AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.*
Fix AI review finding
Some checks failed
Run tests and linters / test (pull_request) Successful in 2m44s
Run tests and linters / lint (pull_request) Failing after 1s
e80f998fb0
Author
Owner

Ready for review and stg testing

There's a lot of linter problems which I'd like to address after review is done as not to add even more noise to already huge PR.

Ready for review and stg testing There's a lot of linter problems which I'd like to address after review is done as not to add even more noise to already huge PR.
Owner

yeah, we need to tackle linting separately.

yeah, we need to tackle linting separately.
update review-voting.md according to review
Some checks failed
Run tests and linters / test (pull_request) Successful in 2m56s
Run tests and linters / lint (pull_request) Failing after 2s
5240987fb2
discussion_sync: update method docs and add a warning
Some checks failed
Run tests and linters / test (pull_request) Successful in 2m53s
Run tests and linters / lint (pull_request) Failing after 1s
9e65d58df3
Implementation details in docstrings are a bad idea, they immediately go
outdated.
setting.py.example: clarify FORGEJO_ADMIN_TEAM
Some checks failed
Run tests and linters / test (pull_request) Successful in 2m53s
Run tests and linters / lint (pull_request) Failing after 1s
b5e8986791
drop remaining references to DISCUSSION_SYSTEM
Some checks failed
Run tests and linters / test (pull_request) Successful in 2m52s
Run tests and linters / lint (pull_request) Failing after 1s
a8f8292192
And some more invalid docs references.
setting.py.example: Document bot access details
Some checks failed
Run tests and linters / test (pull_request) Successful in 2m44s
Run tests and linters / lint (pull_request) Failing after 1s
f04f5df09e
@ -30,23 +30,24 @@ PostgreSQL should be used both for development and production. You can set it up
Run a `Podman`_ container with the latest `PostgreSQL image`_ (replace ``your-password`` with an actual value)::
podman run --detach --name blockerbugsdb \
Owner

I currently have blockerbugsdb (development) and blockerbugs-db (unit tests), which is somewhat confusing. I suggest we use blockerbugsdb (or blockerbugs-db, if you like it better) name for development, and blockerbugs-test-db for testing (which is the same pattern that testdays uses). Sounds OK?

I currently have `blockerbugsdb` (development) and `blockerbugs-db` (unit tests), which is somewhat confusing. I suggest we use `blockerbugsdb` (or `blockerbugs-db`, if you like it better) name for development, and `blockerbugs-test-db` for testing (which is the same pattern that testdays uses). Sounds OK?
Author
Owner

Fixed in ec68cd72cf

Fixed in ec68cd72cf
jgroman marked this conversation as resolved
@ -36,2 +35,3 @@
--env POSTGRESQL_DATABASE=blockerbugs \
--publish 5432:5432 \
docker.io/library/postgres:13
quay.io/fedora/postgresql-13:latest
Owner

CI and unit tests use quay.io/fedora/postgresql-16, I assume this is supposed to say 16 as well?
Btw, do we need to upgrade some OpenShift definition file somewhere as well?

CI and unit tests use `quay.io/fedora/postgresql-16`, I assume this is supposed to say 16 as well? Btw, do we need to upgrade some OpenShift definition file somewhere as well?
Owner

the image deployed to openshift is built by infra/ansible@2db94b4913/roles/openshift-apps/blockerbugs/templates/buildconfig.yml.j2 , AIUI. that uses some sort of magic thing somewhere that knows how to build a python app container from a source repo. I don't think it uses the Dockerfile in the repo at all.

the image deployed to openshift is built by https://forge.fedoraproject.org/infra/ansible/src/commit/2db94b491372a721c22aa159c9e411d2e62a7b23/roles/openshift-apps/blockerbugs/templates/buildconfig.yml.j2 , AIUI. that uses some sort of magic thing somewhere that knows how to build a python app container from a source repo. I don't *think* it uses the Dockerfile in the repo at all.
Owner

oh, but more to the point, we use an external db:

value: "db01{{ env_suffix }}.{{datacenter}}.fedoraproject.org"

        - name: POSTGRESQL_SERVICE_HOST
          value: "db01{{ env_suffix }}.{{datacenter}}.fedoraproject.org"

so we're stuck with whatever postgres version db01 is (in staging and prod). Which, right now, is 16 in both:

[root@db01 ~][PROD-RDU3]# rpm -qa | grep postgres
postgresql-private-libs-16.13-1.module+el9.7.0+24031+188c384c.x86_64
postgresql-16.13-1.module+el9.7.0+24031+188c384c.x86_64
postgresql-server-16.13-1.module+el9.7.0+24031+188c384c.x86_64
postgresql-contrib-16.13-1.module+el9.7.0+24031+188c384c.x86_64
postgresql-plpython3-16.13-1.module+el9.7.0+24031+188c384c.x86_64
postgresql-upgrade-16.13-1.module+el9.7.0+24031+188c384c.x86_64

stg is the same.

oh, but more to the point, we use an external db: https://forge.fedoraproject.org/infra/ansible/src/commit/2db94b491372a721c22aa159c9e411d2e62a7b23/roles/openshift-apps/blockerbugs/templates/deploymentconfig.yml.j2#L50 ``` - name: POSTGRESQL_SERVICE_HOST value: "db01{{ env_suffix }}.{{datacenter}}.fedoraproject.org" ``` so we're stuck with whatever postgres version db01 is (in staging and prod). Which, right now, is 16 in both: ``` [root@db01 ~][PROD-RDU3]# rpm -qa | grep postgres postgresql-private-libs-16.13-1.module+el9.7.0+24031+188c384c.x86_64 postgresql-16.13-1.module+el9.7.0+24031+188c384c.x86_64 postgresql-server-16.13-1.module+el9.7.0+24031+188c384c.x86_64 postgresql-contrib-16.13-1.module+el9.7.0+24031+188c384c.x86_64 postgresql-plpython3-16.13-1.module+el9.7.0+24031+188c384c.x86_64 postgresql-upgrade-16.13-1.module+el9.7.0+24031+188c384c.x86_64 ``` stg is the same.
Author
Owner

Fixed in ec68cd72cf

Fixed in ec68cd72cf
Owner

Thanks for investigation, Adam.

Thanks for investigation, Adam.
jgroman marked this conversation as resolved
@ -0,0 +43,4 @@
VoteCommand = collections.namedtuple("VoteCommand", ["tracker", "vote"])
VoteCommand.__doc__ = (
VoteCommand.__doc__ or ""
) + ': A parsed vote command, e.g. tracker="betablocker", vote="+1"'
Owner

The original code in pagure_bot.py was:

VoteCommand.__doc__ += (
    ': A parsed vote command, e.g. tracker="betablocker", vote="+1"')

I'm not sure exactly why, because we don't seem to use the documentation anywhere. Perhaps the idea was to have the documentation accessible also in runtime and not just through a comment. Why not.

The namedtuple always seem to create an initial doc:

>>> VoteCommand = collections.namedtuple("VoteCommand", ["tracker", "vote"])
>>> VoteCommand.__doc__
'VoteCommand(tracker, vote)'

The new approach is perhaps more defensive but also much harder to read. So why not just do:

VoteCommand.__doc__ = 'VoteCommand(tracker, vote): A parsed vote command, e.g. tracker="betablocker", vote="+1"'

I'll adjust this.

The original code in `pagure_bot.py` was: ``` VoteCommand.__doc__ += ( ': A parsed vote command, e.g. tracker="betablocker", vote="+1"') ``` I'm not sure exactly why, because we don't seem to use the documentation anywhere. Perhaps the idea was to have the documentation accessible also in runtime and not just through a comment. Why not. The `namedtuple` always seem to create an initial doc: ``` >>> VoteCommand = collections.namedtuple("VoteCommand", ["tracker", "vote"]) >>> VoteCommand.__doc__ 'VoteCommand(tracker, vote)' ``` The new approach is perhaps more defensive but also much harder to read. So why not just do: ``` VoteCommand.__doc__ = 'VoteCommand(tracker, vote): A parsed vote command, e.g. tracker="betablocker", vote="+1"' ``` I'll adjust this.
kparal marked this conversation as resolved
@ -0,0 +37,4 @@
)
if not base_url.strip():
raise ForgejoAPIException("FORGEJO_API cannot be empty")
Owner

This whole input arg validation is really chatty, are you used to do it this way? I would personally (at most) do something like (for each arg):

if not base_url or not isinstance(base_url, str) or not base_url.strip():
    raise ValueError(f"FORGEJO_API value invalid, got: {base_url}, type {type(base_url).__name__}")

Maybe I wouldn't even check for type, because it Python you sometimes want to mock objects and give it something that behaves as a string. In the worst case, it will crash during PyforgejoApi initialization a few lines later.

I also chose a general ValueError instead of ForgejoAPIException, because this is not an API call failure. ValueError seems the usual choice when init args are wrong.

Even better to custom checking here (and in similar classes), we could add at least basic checking into the config loader code, so that all of it happens in a central place and all consumers can rely on valid values instead of doing their own checking. But that's not the point of this PR, of course, just saying.

This whole input arg validation is really chatty, are you used to do it this way? I would personally (at most) do something like (for each arg): ``` if not base_url or not isinstance(base_url, str) or not base_url.strip(): raise ValueError(f"FORGEJO_API value invalid, got: {base_url}, type {type(base_url).__name__}") ``` Maybe I wouldn't even check for type, because it Python you sometimes want to mock objects and give it something that _behaves_ as a string. In the worst case, it will crash during `PyforgejoApi` initialization a few lines later. I also chose a general ValueError instead of ForgejoAPIException, because this is not an API call failure. ValueError seems the usual choice when init args are wrong. Even better to custom checking here (and in similar classes), we could add at least basic checking into the config loader code, so that all of it happens in a central place and all consumers can rely on valid values instead of doing their own checking. But that's not the point of this PR, of course, just saying.
Owner

One thing that's bitten me with using generic ValueError before is that, later on, you might at least want to know whether the error is from blockerbugs or something else entirely - I use ValueErrors in fedfind, but have sometimes regretted it in fedfind consumers, where I have to do except ValueError then try to make sure it's the ValueError from fedfind that I was kinda expecting and not some other ValueError.

So - it might be worth having a custom BBValueError that inherits from ValueError, or whatever. Or not...

One thing that's bitten me with using generic `ValueError` before is that, later on, you might at least want to know whether the error is from blockerbugs or something else entirely - I use `ValueError`s in fedfind, but have sometimes regretted it in fedfind consumers, where I have to do `except ValueError` then try to make sure it's the `ValueError` from fedfind that I was kinda expecting and not some *other* `ValueError`. So - it *might* be worth having a custom `BBValueError` that inherits from `ValueError`, or whatever. Or not...
Author
Owner

Fixed in 95205bd802

I'd leave parameter type checking in there for now. It might be useful troubleshooting. We can always remove it later if it causes any problems.

Yes, it would be probably worthwhile to refactor Config into a dataclass in some future update.

Fixed in 95205bd802 I'd leave parameter type checking in there for now. It might be useful troubleshooting. We can always remove it later if it causes any problems. Yes, it would be probably worthwhile to refactor Config into a dataclass in some future update.
Owner

@adamwill wrote in #299 (comment):

One thing that's bitten me with using generic ValueError before is that, later on, you might at least want to know whether the error is from blockerbugs or something else entirely

Interesting. I always like read upon good practices 🙂 So far, it seems to me that in our specific case, the simplicity might be better, because we're not a library, and we only call the code ourselves. But Jaroslav already subclassed it, that's fine, it's just a bit more code to maintain.

@adamwill wrote in https://forge.fedoraproject.org/quality/blockerbugs/pulls/299#issuecomment-594029: > One thing that's bitten me with using generic `ValueError` before is that, later on, you might at least want to know whether the error is from blockerbugs or something else entirely Interesting. I always like read upon good practices 🙂 So far, it seems to me that in our specific case, the simplicity might be better, because we're not a library, and we only call the code ourselves. But Jaroslav already subclassed it, that's fine, it's just a bit more code to maintain.
jgroman marked this conversation as resolved
@ -0,0 +2,4 @@
Testcontainer fixtures (forgejo_container, postgres_container, etc.) have been
moved to testing/conftest.py to make them available for all test modules including
integration tests and end-to-end tests.
Owner

It looks like we can drop this file?

It looks like we can drop this file?
Author
Owner

Fixed in 6438eb98f9

Fixed in 6438eb98f9
jgroman marked this conversation as resolved
forgejo_bot: improve docs
Some checks failed
Run tests and linters / test (pull_request) Successful in 2m43s
Run tests and linters / lint (pull_request) Failing after 1s
1a0d9b1e5e
Make it easier to read and bring back important doc notes that disappeared
Owner

Once we deal with the nitpicks above, I believe this is ready. I have to admit that even though I tried to give it a thorough look, because this PR is very important, I couldn't go through all of it. Especially the test suite-related bits (conftest, test*) are just too long. Combined with lots of changes with the main code base, including formatting, generated docs, etc, and the release validation cycle, it's just all too much. I used to believe in copious documentation and proper docstrings etc, but in the AI age, I'm losing my belief 🙂 It's hard to keep the project in my head when everything is so... spacious. Brevity will be the virtue of the future 😆

Once this is final, I think we don't want to merge it to develop just yet. Instead, we'll want to push this to staging and give it a proper try. That might depend on:

If you know about any further requirements/tasks that need to happen, please say so.

Once we deal with the nitpicks above, I believe this is ready. I have to admit that even though I tried to give it a thorough look, because this PR is very important, I couldn't go through all of it. Especially the test suite-related bits (conftest, test*) are just too long. Combined with lots of changes with the main code base, including formatting, generated docs, etc, and the release validation cycle, it's just all too much. I used to believe in copious documentation and proper docstrings etc, but in the AI age, I'm losing my belief 🙂 It's hard to keep the project in my head when everything is so... spacious. Brevity will be the virtue of the future 😆 Once this is final, I think we don't want to merge it to `develop` just yet. Instead, we'll want to push this to staging and give it a proper try. That might depend on: - https://forge.fedoraproject.org/infra/ansible/pulls/3195 - I'll look at it next - https://forge.fedoraproject.org/forge/forge/issues/477 If you know about any further requirements/tasks that need to happen, please say so.
Rename test containers, update dev db version
Some checks failed
Run tests and linters / test (pull_request) Successful in 3m6s
Run tests and linters / lint (pull_request) Failing after 2s
ec68cd72cf
Consolidate API parameter checking
Some checks failed
Run tests and linters / test (pull_request) Successful in 2m49s
Run tests and linters / lint (pull_request) Failing after 2s
95205bd802
Remove unnecessary file
Some checks failed
Run tests and linters / test (pull_request) Successful in 2m45s
Run tests and linters / lint (pull_request) Failing after 2s
6438eb98f9
Author
Owner

We could probably consider opeaning a feature request for creating a Makefile which would simplify local development - similar to testdays-web setup.

We could probably consider opeaning a feature request for creating a Makefile which would simplify local development - similar to testdays-web setup.
Author
Owner

This can be tested in staging right away. The required env variables can be added and edited in OpenShift console WebUI: Workloads -> Deployments -> blockerbugs -> "..." -> Edit Deployment.

This can be tested in _staging_ right away. The required env variables can be added and edited in OpenShift console WebUI: Workloads -> Deployments -> blockerbugs -> "..." -> Edit Deployment.
Owner

I think that's expected to be done via ansible, like everything else. Keys / passwords / secrets need to be added to the sekrit private ansible thingy on batcave01 and then referenced as variables in the public playbooks, I believe. You can file an infra ticket to get this done - you have to specify what variable names you want to exist, and if the values need to be specific we have to set up some kind of secure channel to transfer them; if infra can just make them up, that's easier.

I think that's expected to be done via ansible, like everything else. Keys / passwords / secrets need to be added to the sekrit private ansible thingy on batcave01 and then referenced as variables in the public playbooks, I believe. You can file an infra ticket to get this done - you have to specify what variable names you want to exist, and if the values need to be specific we have to set up some kind of secure channel to transfer them; if infra can just make them up, that's easier.
Author
Owner

I mean the infra ticket is already in the works and linked from #296 but until it is done we can just tweak env vars manually. We do not have to wait for it to be reviewed and merged.

I mean the infra ticket is already in the works and linked from #296 but until it is done we can just tweak env vars manually. We do not have to wait for it to be reviewed and merged.
development.rst: Fix one more reference to Postgresql image version
Some checks failed
Run tests and linters / test (pull_request) Successful in 2m46s
Run tests and linters / lint (pull_request) Failing after 1s
ad630fd297
Owner

I've started focusing on infra/ansible#3195 . There also seems to be some progress in forge/forge#477 (although no replies yet), so hopefully this won't take long anymore.

I've started focusing on https://forge.fedoraproject.org/infra/ansible/pulls/3195 . There also seems to be some progress in https://forge.fedoraproject.org/forge/forge/issues/477 (although no replies yet), so hopefully this won't take long anymore.
Owner

forge/forge#477 is now done, we should be fully capable of testing this in stg env without any workarounds. Jaroslav, have you tried?
(I believe we need to first update the staging branch tip and redeploy in openshift, though).

https://forge.fedoraproject.org/forge/forge/issues/477 is now done, we should be fully capable of testing this in stg env without any workarounds. Jaroslav, have you tried? (I believe we need to first update the `staging` branch tip and redeploy in openshift, though).
Author
Owner

I missed the info regarding forge/forge#477 being done so no testing yet. Will focus on that now.

I missed the info regarding forge/forge#477 being done so no testing yet. Will focus on that now.
Owner

Let me know if you need stg FAS groups adjustments, I haven't checked those yet.

Let me know if you need stg FAS groups adjustments, I haven't checked those yet.
Owner

@jgroman Based on #296 (comment) , you can please adjust the code to check Quality org membership instead of Quality Members team membership?

@jgroman Based on https://forge.fedoraproject.org/quality/blockerbugs/issues/296#issuecomment-676611 , you can please adjust the code to check _Quality org membership_ instead of _Quality Members team membership_?
kparal left a comment
Owner

Requesting changes requires a reason (boo!), and I always realize only after providing a regular comment instead 🙂

Requesting changes requires a reason (boo!), and I always realize only after providing a regular comment instead 🙂️
Implement admin rights based on org public membership
Some checks failed
Run tests and linters / test (pull_request) Failing after 3m3s
Run tests and linters / lint (pull_request) Has been skipped
950293a229
Owner

Sooo... how angry will you be if I propose the drop the caching feature that you just spent time to implement? 😬️ I know the old comment requested it... But thinking about it, we use AGREED or similar a few times per day at most. That's zero extra load for Forge. Caching means we get an extra code to maintain, and an extra configuration option. I think it's not quite worth it, I'd rather keep it simpler. Sorry! Wdyt?

Sooo... how angry will you be if I propose the drop the caching feature that you just spent time to implement? 😬️ I know the old comment requested it... But thinking about it, we use AGREED or similar a few times per day at most. That's zero extra load for Forge. Caching means we get an extra code to maintain, and an extra configuration option. I think it's not quite worth it, I'd rather keep it simpler. Sorry! Wdyt?
Author
Owner

OK, no problem with that!

OK, no problem with that!
Remove admin member caching
Some checks failed
Run tests and linters / test (pull_request) Failing after 2m35s
Run tests and linters / lint (pull_request) Has been skipped
3bb0fcf923
Fix sending unauthenticated API request
Some checks failed
Run tests and linters / test (pull_request) Failing after 2m33s
Run tests and linters / lint (pull_request) Has been skipped
16ec640ff4
Owner

This seems to be working properly now in our quick testing in staging. A 🫙️ of 🍪️ for @jgroman !

I'll merge this PR, and we can follow up with the remaining steps in #296.

This seems to be working properly now in our quick testing in staging. A 🫙️ of 🍪️ for @jgroman ! I'll merge this PR, and we can follow up with the remaining steps in #296.
kparal force-pushed fix/296-blocker-review-voting-on-forge from 16ec640ff4
Some checks failed
Run tests and linters / test (pull_request) Failing after 2m33s
Run tests and linters / lint (pull_request) Has been skipped
to dc63589bdd
Some checks failed
Run tests and linters / test (pull_request) Failing after 2m51s
Run tests and linters / lint (pull_request) Has been skipped
2026-04-29 14:02:45 +00:00
Compare
Fix tests failing due to publicize problem
Some checks failed
Run tests and linters / test (pull_request) Successful in 2m49s
Run tests and linters / lint (pull_request) Failing after 2s
62bd4242e1
kparal force-pushed fix/296-blocker-review-voting-on-forge from 62bd4242e1
Some checks failed
Run tests and linters / test (pull_request) Successful in 2m49s
Run tests and linters / lint (pull_request) Failing after 2s
to 5a6f2b5736
Some checks failed
Run tests and linters / test (pull_request) Successful in 3m13s
Run tests and linters / lint (pull_request) Failing after 2s
2026-04-29 14:28:03 +00:00
Compare
jgroman manually merged commit 5a6f2b5736 into develop 2026-04-29 14:33:53 +00:00
kparal deleted branch fix/296-blocker-review-voting-on-forge 2026-04-29 14:34:12 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference
quality/blockerbugs!299
No description provided.