Fix bridge bug mirroring failures in ocp4-scan-konflux - #3202
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughJIRA issue creation now validates configured versions against project versions, while bridge payloads omit empty version fields. Bridge bug mirroring contains setup, execution, and Slack notification failures, with tests covering configuration and notification errors. ChangesJIRA and bridge issue handling
Bridge mirroring resilience
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Test build (VERSION=4.23, ART_TOOLS_COMMIT=joepvd@7af8892): https://art-jenkins.apps.prod-stable-spoke1-dc-iad2.itup.redhat.com/job/aos-cd-builds/job/build%252Focp4-scan-konflux/54791/ |
|
Retry test build (VERSION=4.23, ART_TOOLS_COMMIT=joepvd@fix-bridge-bug-mirroring): https://art-jenkins.apps.prod-stable-spoke1-dc-iad2.itup.redhat.com/job/aos-cd-builds/job/build%252Focp4-scan-konflux/54792/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyartcd/pyartcd/pipelines/ocp4_scan_konflux.py`:
- Around line 310-318: Update the exception handlers in the bridge bug mirroring
failure flow to stop logging raw exception messages or tracebacks that may
expose sensitive details. In the handler around the bridge mirroring operation
and the nested Slack notification handler, log only a sanitized exception type
(such as the exception class name) while preserving the existing failure
handling and notification behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a0518b3a-0956-4e94-a555-d754b2c1efff
📒 Files selected for processing (3)
elliott/elliottlib/cli/find_bugs_bridge_cli.pypyartcd/pyartcd/pipelines/ocp4_scan_konflux.pypyartcd/tests/pipelines/test_ocp4_scan_konflux.py
💤 Files with no reviewable changes (1)
- elliott/elliottlib/cli/find_bugs_bridge_cli.py
7af8892 to
f1e63b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pyartcd/tests/pipelines/test_ocp4_scan_konflux.py (1)
221-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the injected failure paths were exercised.
These tests only assert that the handler returns. Add
assert_awaited_once()forload_group_config/cmd_assert_asyncandassert_called_once_with()fornew_slack_client; otherwise a regression that skips mirroring or notification would still pass.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyartcd/tests/pipelines/test_ocp4_scan_konflux.py` around lines 221 - 236, Update test_handle_bridge_bug_mirroring_does_not_fail_on_slack_error to assert load_group_config and cmd_assert_async were each awaited once, and assert runtime.new_slack_client was called once with its expected arguments, confirming both mirroring and notification failure paths were exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyartcd/tests/pipelines/test_ocp4_scan_konflux.py`:
- Around line 215-220: Update
test_handle_bridge_bug_mirroring_does_not_fail_on_config_error to override the
Slack client returned by runtime.new_slack_client, using a synchronous MagicMock
for bind_channel and an async mock for say, matching the failure test setup.
Keep the config-loading RuntimeError scenario and ensure no un-awaited coroutine
is created by handle_bridge_bug_mirroring.
---
Nitpick comments:
In `@pyartcd/tests/pipelines/test_ocp4_scan_konflux.py`:
- Around line 221-236: Update
test_handle_bridge_bug_mirroring_does_not_fail_on_slack_error to assert
load_group_config and cmd_assert_async were each awaited once, and assert
runtime.new_slack_client was called once with its expected arguments, confirming
both mirroring and notification failure paths were exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9a848a3b-b917-45cf-b114-ba931428c524
📒 Files selected for processing (2)
pyartcd/pyartcd/pipelines/ocp4_scan_konflux.pypyartcd/tests/pipelines/test_ocp4_scan_konflux.py
f1e63b2 to
68abf8a
Compare
|
Test build #54793 (with version filtering fix): https://art-jenkins.apps.prod-stable-spoke1-dc-iad2.itup.redhat.com/job/aos-cd-builds/job/build%252Focp4-scan-konflux/54793/console |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
elliott/elliottlib/bzutil.py (2)
992-993: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache project versions across issue creations.
create_issueperforms a remoteproject_versionslookup for every issue with configured versions. Batch mirroring can therefore generate redundant JIRA requests and amplify latency or rate-limit failures. Cache successful lookups onJIRABugTracker, while allowing transient failures to retry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@elliott/elliottlib/bzutil.py` around lines 992 - 993, Update JIRABugTracker’s create_issue version-resolution flow around self._client.project_versions(self.project) to cache successful project-version lookups and reuse them across issue creations. Store the cache on the tracker instance, but only populate it after a successful remote lookup so transient failures remain retryable; preserve the existing behavior when versions are configured.
992-996: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDo not swallow every exception as a lookup failure.
except Exceptionalso hides programming errors such as malformed JIRA version objects, then returns the unvalidated configured list and bypasses the new allow-list. Catch only the expected JIRA/transport failures and re-raise unexpected exceptions; verify the exact exception hierarchy forjira3.10.5.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@elliott/elliottlib/bzutil.py` around lines 992 - 996, Update the exception handling around self._client.project_versions in the project-version lookup to catch only the specific JIRA/transport exception types supported by jira 3.10.5. Preserve the fallback to configured versions for those expected failures, while allowing malformed version data and all other unexpected exceptions to propagate instead of bypassing validation.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@elliott/elliottlib/bzutil.py`:
- Around line 992-993: Update JIRABugTracker’s create_issue version-resolution
flow around self._client.project_versions(self.project) to cache successful
project-version lookups and reuse them across issue creations. Store the cache
on the tracker instance, but only populate it after a successful remote lookup
so transient failures remain retryable; preserve the existing behavior when
versions are configured.
- Around line 992-996: Update the exception handling around
self._client.project_versions in the project-version lookup to catch only the
specific JIRA/transport exception types supported by jira 3.10.5. Preserve the
fallback to configured versions for those expected failures, while allowing
malformed version data and all other unexpected exceptions to propagate instead
of bypassing validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5cf6cfff-50c6-4897-89e5-43c75db213ed
📒 Files selected for processing (4)
elliott/elliottlib/bzutil.pyelliott/elliottlib/cli/find_bugs_bridge_cli.pypyartcd/pyartcd/pipelines/ocp4_scan_konflux.pypyartcd/tests/pipelines/test_ocp4_scan_konflux.py
💤 Files with no reviewable changes (1)
- elliott/elliottlib/cli/find_bugs_bridge_cli.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pyartcd/tests/pipelines/test_ocp4_scan_konflux.py
68abf8a to
bd15fd0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
elliott/elliottlib/bzutil.py (1)
1184-1192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid mutating the process-wide Jira logger for one call.
setLevel(logging.ERROR)suppresses every warning fromjira.client, not just the known link-type message. If mirror tasks overlap, interleaved save/restore operations can also leave the logger at an incorrect level. Prefer filtering the specific message or verify that link creation is strictly serialized for thejiraclient version in use.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@elliott/elliottlib/bzutil.py` around lines 1184 - 1192, Update the link-creation flow around self._client.create_issue_link to avoid changing the process-wide jira.client logger level. Prefer a temporary filter that suppresses only the known “Specified issue link type is not present” message while preserving all other warnings; otherwise, ensure calls are strictly serialized with safe restoration for the Jira client version in use.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@elliott/elliottlib/bzutil.py`:
- Around line 993-997: Update _filter_valid_versions to catch only the specific
Jira client or network exception types that represent recoverable
project-version fetch failures, rather than broad Exception. Preserve returning
the configured versions for those handled failures, and include traceback
details in the logger.warning call; allow unexpected errors and malformed
responses to propagate.
- Around line 968-970: Cache the filtered project-version names on
JIRABugTracker before bridge issue creation, then update create_issue’s versions
handling to reuse that cache instead of calling project_versions() for each
issue when version is configured. Preserve the existing filtering and
fields['versions'] construction, and ensure the cache is populated once per
relevant Jira project.
---
Nitpick comments:
In `@elliott/elliottlib/bzutil.py`:
- Around line 1184-1192: Update the link-creation flow around
self._client.create_issue_link to avoid changing the process-wide jira.client
logger level. Prefer a temporary filter that suppresses only the known
“Specified issue link type is not present” message while preserving all other
warnings; otherwise, ensure calls are strictly serialized with safe restoration
for the Jira client version in use.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ec06bea0-91b3-4e34-86f3-4018022eca0d
📒 Files selected for processing (4)
elliott/elliottlib/bzutil.pyelliott/elliottlib/cli/find_bugs_bridge_cli.pypyartcd/pyartcd/pipelines/ocp4_scan_konflux.pypyartcd/tests/pipelines/test_ocp4_scan_konflux.py
💤 Files with no reviewable changes (1)
- elliott/elliottlib/cli/find_bugs_bridge_cli.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pyartcd/tests/pipelines/test_ocp4_scan_konflux.py
| try: | ||
| project_versions = {v.name for v in self._client.project_versions(self.project)} | ||
| except Exception: | ||
| logger.warning("Could not fetch project versions for %s; using configured versions as-is", self.project) | ||
| return versions |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file around the reported lines
sed -n '930,1035p' elliott/elliottlib/bzutil.py
# Find where project_versions is defined or used
rg -n "project_versions\(" elliott -S
# Find create_issue call path / version handling around this helper
rg -n "create_issue|_filter_valid_versions|versions as-is|using configured versions" elliott -S
# Show nearby definitions and imports for exception handling context
sed -n '1,140p' elliott/elliottlib/bzutil.pyRepository: openshift-eng/art-tools
Length of output: 12960
🌐 Web query:
python-jira project_versions exceptions JIRAError requests.exceptions documentation
💡 Result:
In the python-jira library, project versions are accessed via the project_versions method, which belongs to the JIRA client class [1][2][3]. The primary exception raised by the library is JIRAError, which is defined in the jira.exceptions module [4][5][6]. JIRAError is a general exception raised for problems encountered during client operations [4]. It contains attributes such as status_code, text, url, request, and response, which can be useful for debugging when an API request fails [4]. Because the python-jira library uses the requests library under the hood for HTTP communication, you may also encounter exceptions from requests (e.g., requests.exceptions.RequestException or connection-related errors) [4][6][7]. It is best practice to handle these exceptions separately if you need to distinguish between library-specific logic errors (JIRAError) and underlying transport or network issues (requests.exceptions) [6][7]. Example usage: from jira import JIRA from jira.exceptions import JIRAError import requests jira = JIRA('https://your-jira-instance.com', auth=('user', 'pass')) try: versions = jira.project_versions('PROJECT_KEY') for version in versions: print(version.name) except JIRAError as e: print(f"JIRA API error: {e.status_code} - {e.text}") except requests.exceptions.RequestException as e: print(f"Network or request error: {e}") except Exception as e: print(f"An unexpected error occurred: {e}")
Citations:
- 1: https://jira.readthedocs.io/api.html
- 2: https://jira.readthedocs.io/examples.html
- 3: http://jira.readthedocs.org/genindex.html
- 4: https://jira.readthedocs.io/_modules/jira/exceptions.html
- 5: https://jira.readthedocs.io/
- 6: https://jira.readthedocs.io/_modules/jira/client.html
- 7: https://github.com/pycontribs/jira/blob/3158d5948ff251a4e13fc48bea88cbf79339b3bc/jira/client.py
Narrow the fallback in _filter_valid_versions Catch only the Jira client/network exceptions you can recover from instead of Exception, and include the traceback in the warning so unexpected bugs and malformed responses still surface.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 995-995: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@elliott/elliottlib/bzutil.py` around lines 993 - 997, Update
_filter_valid_versions to catch only the specific Jira client or network
exception types that represent recoverable project-version fetch failures,
rather than broad Exception. Preserve returning the configured versions for
those handled failures, and include traceback details in the logger.warning
call; allow unexpected errors and malformed responses to propagate.
Source: Linters/SAST tools
The _build_issue_fields method was explicitly setting versions to an empty list, which prevented bzutil.create_issue from applying the tracker's configured default versions. Jira's OCPBUGS project requires a non-empty Affects Versions field, causing issue creation to fail with HTTP 400. Remove the explicit empty assignment so create_issue fills in the tracker's configured versions automatically. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Broaden the exception handling in handle_bridge_bug_mirroring to catch all exceptions, not just ChildProcessError from the elliott subprocess. This prevents failures in config loading or Slack notification from blocking the scan pipeline. The Slack notification is also wrapped in its own try/except to avoid masking the original error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bd15fd0 to
8ac2a66
Compare
|
@joepvd: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
thegreyd
added
the
tide/merge-method-squash
| if versions and 'versions' not in fields: | ||
| fields['versions'] = [{'name': version} for version in versions] | ||
| versions = self._filter_valid_versions(versions) | ||
| if versions: |
There was a problem hiding this comment.
_filter_valid_versions() can return an empty list, after which create_issue() simply omits versions. Since Affects Version is required, this can still result in HTTP 400—just with “Affects versions is required.” It could fail early with an actionable ValueError, or deliberately select a valid fallback.
| # upstream jira library — it fires even when the link type name is correct. | ||
| jira_logger = logging.getLogger("jira.client") | ||
| prev_level = jira_logger.level | ||
| jira_logger.setLevel(logging.ERROR) |
There was a problem hiding this comment.
create_issue_link() temporarily changes the global jira.client logger level. Concurrent calls can restore levels in the wrong order, potentially leaving Jira logging permanently suppressed. This could be replaced with a narrowly scoped logging filter.
| mock_issue = flexmock(key="OCPBUGS-1") | ||
| mock_jira_client = flexmock() | ||
| mock_version = flexmock(name="4.23") | ||
| mock_jira_client.should_receive("project_versions").with_args("OCPBUGS").and_return([mock_version]) |
There was a problem hiding this comment.
The existing default-field test was adjusted, but there are no tests covering partially invalid versions, all-invalid versions, project_versions() failure, or cache behavior. These cases could be added as regression coverage.
| except ChildProcessError as e: | ||
|
|
||
| except Exception as e: | ||
| self.logger.error("Bridge bug mirroring failed for %s: %s", self.version, e) |
There was a problem hiding this comment.
handle_bridge_bug_mirroring() now catches every Exception, but records only logger.error(... %s, e). Because the error is intentionally non-fatal, the log is the primary diagnostic evidence. It could use logger.exception(...) to preserve the traceback.
| fields['versions'] = [{'name': version} for version in versions] | ||
| if target_releases is None: | ||
| target_releases = self.target_release() | ||
| if target_releases and self.field_target_version not in fields: |
There was a problem hiding this comment.
Target Version validation currently uses the values available to Jira’s Bug issue type, while bridge mirrors preserve the source issue type and may create a Story or another type. Since Jira field contexts can vary by issue type, a target version can pass Elliott’s initial validation but still be rejected during creation. This is visible in ocp4-scan-konflux build 54792, where 4.23.z is excluded from the query but issue creation subsequently fails with Version name '4.23.z' is not valid. Could we extend this change to validate target_releases against the Target Version values allowed for the actual issue type in fields?
2 participants
Summary
_build_issue_fieldswas settingversionsto an empty list, preventingcreate_issuefrom applying the tracker's configured Affects Versions. Jira rejected the creation with HTTP 400 ("Affects versions is required").ChildProcessError), so failures in config loading, the elliott command, or Slack notification cannot block the scan.Test plan
pytest elliott/tests/test_find_bugs_bridge_cli.py— 12 tests passpytest pyartcd/tests/pipelines/test_ocp4_scan_konflux.py— 10 tests pass (including 2 new tests for broader error handling)🤖 Generated with Claude Code
Summary by CodeRabbit