Skip to content

Enable setting of reviewers in Ymir's MRs - #723

Merged
lbarcziova merged 1 commit into
packit:mainfrom
lbarcziova:assign-reviewers-enable
Jul 30, 2026
Merged

lbarcziova merged 1 commit into
packit:mainfrom
lbarcziova:assign-reviewers-enable

Conversation

@lbarcziova

Copy link
Copy Markdown
Member

No description provided.

@qodo-for-packit

Copy link
Copy Markdown

PR Summary by Qodo

Enable automatic reviewer assignment for new Ymir merge requests

⚙️ Configuration changes ✨ Enhancement 🕐 Less than 10 minutes

Grey Divider

AI Description

• Enable reviewer assignment for newly created merge requests via ASSIGN_MR_REVIEWERS.
• Drive reviewer selection from Bugzilla component contacts (Default Assignee + QA Contact).
• Roll out via OpenShift ConfigMap so behavior can be toggled per deployment.
Diagram

graph TD
  CM["OpenShift ConfigMap"] --> AG["Ymir agents"] --> GL["GitLab MR API"] --> MR(["Merge request"])
  AG --> BZ{{"Bugzilla component data"}}

  subgraph Legend
    direction LR
    _cfg["Config"] ~~~ _svc["Service"] ~~~ _ext{{"External"}} ~~~ _obj(["Artifact"])
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Default to disabled; enable per-namespace after validation
  • ➕ Safer rollout if reviewer resolution or GitLab permissions are uncertain
  • ➕ Avoids unexpected reviewer pings/spam during initial deployment
  • ➖ Delays benefit for users until toggled on
  • ➖ Requires an extra ops step to enable everywhere
2. Per-agent / per-workflow toggle (instead of global ConfigMap)
  • ➕ More granular control (e.g., only enable for certain agent types or projects)
  • ➕ Easier to A/B test behavior
  • ➖ Adds configuration complexity and more places to reason about behavior
  • ➖ Harder to communicate a single source of truth for enablement

Recommendation: Keeping this as a deployment-time feature flag in the OpenShift ConfigMap is the right trade-off for simple, centralized control. If this is the first time enabling it in production, consider a staged rollout (environment-by-environment) to validate reviewer resolution and GitLab permissions before enabling globally.

Files changed (1) +3 / -0

Other (1) +3 / -0
configmap-agents-env.ymlEnable ASSIGN_MR_REVIEWERS feature flag for Ymir agents +3/-0

Enable ASSIGN_MR_REVIEWERS feature flag for Ymir agents

• Adds the ASSIGN_MR_REVIEWERS environment variable (set to "true") with documentation describing reviewer assignment based on Bugzilla component contacts. This turns on automatic reviewer assignment for newly created merge requests in this deployment.

openshift/configmap-agents-env.yml

@qodo-for-packit

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 7 rules

Grey Divider


Remediation recommended

1. Reviewer assignment blocks workflow 🐞 Bug ➹ Performance
Description
Enabling ASSIGN_MR_REVIEWERS makes newly-created MR flows await reviewer resolution and assignment,
which can add noticeable latency when GitLab/bugzilla-data/LDAP are slow (multiple calls have 30s
timeouts). This can extend the critical path of MR creation for every new MR and increase the chance
of step-level timeouts under transient network/LDAP issues.
Code

openshift/configmap-agents-env.yml[R21-23]

+  # When "true", agents assign reviewers to newly created MRs based on
+  # the package's bugzilla component contacts (Default Assignee + QA Contact).
+  ASSIGN_MR_REVIEWERS: "true"
Relevance

●●● Strong

Team previously accepted reverting risky global ConfigMap toggles that add operational latency/risk
until logic is improved.

PR-#657

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The ConfigMap change enables the feature globally; the MR creation helper awaits reviewer assignment
for new MRs; reviewer resolution performs network/LDAP operations with explicit 30s timeouts, so
enabling it can add latency to the MR creation critical path.

openshift/configmap-agents-env.yml[20-24]
ymir/agents/tasks.py[291-379]
ymir/tools/constants.py[1-6]
ymir/tools/privileged/reviewer_resolver.py[36-75]
ymir/tools/privileged/reviewer_resolver.py[156-203]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ASSIGN_MR_REVIEWERS` is enabled in the cluster ConfigMap, which activates `request_mr_reviews()` on every newly-created MR. That function performs multiple network requests (bugzilla-data fetch, GitLab user lookups, and potentially an LDAP lookup) with 30s timeouts, and it is currently awaited in the MR creation path, adding avoidable latency.

### Issue Context
Reviewer assignment is already best-effort (exceptions are swallowed), so it can be safely moved off the critical path without changing correctness of the core MR creation workflow.

### Fix Focus Areas
- openshift/configmap-agents-env.yml[21-23]
- ymir/agents/tasks.py[291-379]
- ymir/tools/constants.py[1-6]
- ymir/tools/privileged/reviewer_resolver.py[36-75]
- ymir/tools/privileged/reviewer_resolver.py[156-203]

### Suggested change
- In `ymir/agents/tasks.py`, avoid blocking MR creation on reviewer assignment (e.g., `asyncio.create_task(request_mr_reviews(...))` with an error-logging done-callback), *or* gate the LDAP fallback / reduce timeouts so the worst-case delay is bounded.
- If you need a safer rollout, consider leaving `ASSIGN_MR_REVIEWERS` disabled by default in the ConfigMap and enabling it per-environment/canary once latency and dependencies (LDAP/Kerberos) are confirmed.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

2. Reviewer errors lack traceback 🐞 Bug ◔ Observability
Description
With ASSIGN_MR_REVIEWERS enabled, failures during reviewer resolution/assignment will be logged
without a stack trace in request_mr_reviews, making production failures harder to diagnose. This
reduces observability exactly on a newly-activated integration path (bugzilla-data, GitLab user
lookup, LDAP).
Code

openshift/configmap-agents-env.yml[R21-23]

+  # When "true", agents assign reviewers to newly created MRs based on
+  # the package's bugzilla component contacts (Default Assignee + QA Contact).
+  ASSIGN_MR_REVIEWERS: "true"
Relevance

●●● Strong

Repo commonly uses logger.exception/tracebacks for operational failures; adding exc_info is a
low-risk observability improvement.

PR-#675

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The ConfigMap enables reviewer assignment globally, and the implementation catches all exceptions
but logs without exc_info=True, so failures will be hard to debug once the path is exercised in
production.

openshift/configmap-agents-env.yml[20-24]
ymir/agents/tasks.py[291-319]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
When reviewer assignment fails, `request_mr_reviews()` logs only the exception message, not the traceback. With `ASSIGN_MR_REVIEWERS` enabled by default, this will make it harder to identify whether failures come from bugzilla-data fetch, GitLab API calls, or LDAP/Kerberos.

### Issue Context
The function is explicitly best-effort (it should not raise), so improving logs is the main way to debug real failures.

### Fix Focus Areas
- ymir/agents/tasks.py[291-319]
- openshift/configmap-agents-env.yml[21-23]

### Suggested change
- Change the exception handler to include traceback details (e.g., `logger.warning("...", exc_info=True)`), and optionally include which tool call failed (resolve vs set reviewers) to speed up diagnosis.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

Comment on lines +21 to +23
# When "true", agents assign reviewers to newly created MRs based on
# the package's bugzilla component contacts (Default Assignee + QA Contact).
ASSIGN_MR_REVIEWERS: "true"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

1. Reviewer assignment blocks workflow 🐞 Bug ➹ Performance

Enabling ASSIGN_MR_REVIEWERS makes newly-created MR flows await reviewer resolution and assignment,
which can add noticeable latency when GitLab/bugzilla-data/LDAP are slow (multiple calls have 30s
timeouts). This can extend the critical path of MR creation for every new MR and increase the chance
of step-level timeouts under transient network/LDAP issues.
Agent Prompt
### Issue description
`ASSIGN_MR_REVIEWERS` is enabled in the cluster ConfigMap, which activates `request_mr_reviews()` on every newly-created MR. That function performs multiple network requests (bugzilla-data fetch, GitLab user lookups, and potentially an LDAP lookup) with 30s timeouts, and it is currently awaited in the MR creation path, adding avoidable latency.

### Issue Context
Reviewer assignment is already best-effort (exceptions are swallowed), so it can be safely moved off the critical path without changing correctness of the core MR creation workflow.

### Fix Focus Areas
- openshift/configmap-agents-env.yml[21-23]
- ymir/agents/tasks.py[291-379]
- ymir/tools/constants.py[1-6]
- ymir/tools/privileged/reviewer_resolver.py[36-75]
- ymir/tools/privileged/reviewer_resolver.py[156-203]

### Suggested change
- In `ymir/agents/tasks.py`, avoid blocking MR creation on reviewer assignment (e.g., `asyncio.create_task(request_mr_reviews(...))` with an error-logging done-callback), *or* gate the LDAP fallback / reduce timeouts so the worst-case delay is bounded.
- If you need a safer rollout, consider leaving `ASSIGN_MR_REVIEWERS` disabled by default in the ConfigMap and enabling it per-environment/canary once latency and dependencies (LDAP/Kerberos) are confirmed.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines +21 to +23
# When "true", agents assign reviewers to newly created MRs based on
# the package's bugzilla component contacts (Default Assignee + QA Contact).
ASSIGN_MR_REVIEWERS: "true"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Informational

2. Reviewer errors lack traceback 🐞 Bug ◔ Observability

With ASSIGN_MR_REVIEWERS enabled, failures during reviewer resolution/assignment will be logged
without a stack trace in request_mr_reviews, making production failures harder to diagnose. This
reduces observability exactly on a newly-activated integration path (bugzilla-data, GitLab user
lookup, LDAP).
Agent Prompt
### Issue description
When reviewer assignment fails, `request_mr_reviews()` logs only the exception message, not the traceback. With `ASSIGN_MR_REVIEWERS` enabled by default, this will make it harder to identify whether failures come from bugzilla-data fetch, GitLab API calls, or LDAP/Kerberos.

### Issue Context
The function is explicitly best-effort (it should not raise), so improving logs is the main way to debug real failures.

### Fix Focus Areas
- ymir/agents/tasks.py[291-319]
- openshift/configmap-agents-env.yml[21-23]

### Suggested change
- Change the exception handler to include traceback details (e.g., `logger.warning("...", exc_info=True)`), and optionally include which tool call failed (resolve vs set reviewers) to speed up diagnosis.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@lbarcziova
lbarcziova merged commit 5a639c5 into packit:main Jul 30, 2026
11 checks passed
@lbarcziova
lbarcziova deleted the assign-reviewers-enable branch July 30, 2026 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants