OCPERT-349: Add job command for running Stage testing prow job#969
OCPERT-349: Add job command for running Stage testing prow job#969tomasdavidorg wants to merge 2 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@tomasdavidorg: This pull request references OCPERT-349 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this: Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds documentation for two new CLI commands and implements stage-testing job support: payload URL validation, minor-release extraction, Gangway job submission, response handling, and result polling; also exposes a new CLI command. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@prow/job/job.py`:
- Around line 440-444: The Gangway POST call at job_run_res =
self._get_session().post(...) has no timeout and can hang; add a timeout kwarg
to the post call (for example timeout=self.request_timeout or timeout=10) so the
request will fail fast on network stalls, and if you choose a configurable value
add a request_timeout attribute (or a constant) to the class and use that in the
call; update the post invocation in job_run_res to include
timeout=<configured_value>.
In `@prow/README.md`:
- Around line 93-97: The README wording "shipment MR" is inconsistent with the
CLI option; update the README example and description for the
run_image_consistency_check command to refer to the merge request ID and the
actual flag name used by the CLI (--mr-id / -m), e.g., change "shipment MR" to
"merge request ID (MR) via -m/--mr-id" and ensure the sample invocation and
explanatory text match the option implemented in the job
run_image_consistency_check command and the CLI option --mr-id in
prow/job/job.py.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71d18871-07d9-4be6-929c-f57cd920fc02
📒 Files selected for processing (2)
prow/README.mdprow/job/job.py
| job_run_res = self._get_session().post( | ||
| url=url, | ||
| json=data, | ||
| headers=self.get_prow_headers() | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import re
from pathlib import Path
p = Path("prow/job/job.py")
text = p.read_text(encoding="utf-8")
m = re.search(r"def run_stage_testing\(.*?return job_status", text, re.S)
if not m:
raise SystemExit("run_stage_testing block not found")
block = m.group(0)
print("timeout_present_in_run_stage_testing_post =", "timeout=" in block)
for i, line in enumerate(block.splitlines(), 1):
if ".post(" in line or "timeout=" in line:
print(f"{i:03d}: {line}")
PYRepository: openshift/release-tests
Length of output: 169
🏁 Script executed:
#!/bin/bash
# Verify lines 440-444 match the review comment
head -444 prow/job/job.py | tail -20Repository: openshift/release-tests
Length of output: 695
🏁 Script executed:
#!/bin/bash
# Search for all .post() calls in the file to check for other timeout issues
rg "\.post\(" prow/job/job.py -A 3 -B 2Repository: openshift/release-tests
Length of output: 650
🏁 Script executed:
#!/bin/bash
# Check for any session configuration or timeout defaults in the file
rg "session|timeout" prow/job/job.py -i | head -20Repository: openshift/release-tests
Length of output: 1183
Add a timeout to the Gangway POST request.
The POST call to the Gangway API (lines 440-444) currently has no timeout, so the CLI can block indefinitely on network/API stalls.
job_run_res = self._get_session().post(
url=url,
json=data,
- headers=self.get_prow_headers()
+ headers=self.get_prow_headers(),
+ timeout=30
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| job_run_res = self._get_session().post( | |
| url=url, | |
| json=data, | |
| headers=self.get_prow_headers() | |
| ) | |
| job_run_res = self._get_session().post( | |
| url=url, | |
| json=data, | |
| headers=self.get_prow_headers(), | |
| timeout=30 | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@prow/job/job.py` around lines 440 - 444, The Gangway POST call at job_run_res
= self._get_session().post(...) has no timeout and can hang; add a timeout kwarg
to the post call (for example timeout=self.request_timeout or timeout=10) so the
request will fail fast on network stalls, and if you choose a configurable value
add a request_timeout attribute (or a constant) to the class and use that in the
call; update the post invocation in job_run_res to include
timeout=<configured_value>.
| Run the image consistency check Prow job for a given release payload and shipment MR. | ||
|
|
||
| ```console | ||
| $ job run_image_consistency_check -p quay.io/openshift-release-dev/ocp-release:4.19.1-x86_64 -m 189 | ||
| Image consistency check job id: a1b2c3d4-e5f6-7890-abcd-ef1234567890 |
There was a problem hiding this comment.
Align MR argument wording with the implemented CLI option.
Line 93 says “shipment MR”, but the command interface uses merge request ID via -m/--mr-id (prow/job/job.py, Line 816). Please align wording/flag naming in README to prevent confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@prow/README.md` around lines 93 - 97, The README wording "shipment MR" is
inconsistent with the CLI option; update the README example and description for
the run_image_consistency_check command to refer to the merge request ID and the
actual flag name used by the CLI (--mr-id / -m), e.g., change "shipment MR" to
"merge request ID (MR) via -m/--mr-id" and ensure the sample invocation and
explanatory text match the option implemented in the job
run_image_consistency_check command and the CLI option --mr-id in
prow/job/job.py.
| @@ -350,7 +351,11 @@ def _is_valid_payload_url(self, payload_url: str) -> bool: | |||
| pattern = r"^quay\.io/openshift-release-dev/ocp-release:\d+\.\d+\.\d+.*-x86_64$" | |||
| return re.match(pattern, payload_url) is not None | |||
|
|
|||
There was a problem hiding this comment.
Remove trailing whitespace on this line for consistency with project style.
| def _get_minor_release_from_payload_url(self, payload_url): | ||
| """Extract minor release (e.g. '4.19') from a validated payload URL.""" | ||
| z_version = payload_url.split(":")[1].split("-")[0] | ||
| return ".".join(z_version.split(".")[:2]) |
There was a problem hiding this comment.
Since you've extracted the minor release parsing logic into _get_minor_release_from_payload_url(), consider refactoring run_image_consistency_check() to use this same helper method instead of duplicating the logic.
This would eliminate duplication and make the code more maintainable. For example, you could replace the inline parsing in run_image_consistency_check() with a call to this helper.
There was a problem hiding this comment.
There is no duplicated parsing in run_image_consistency_check(). It uses a single constant job name (IMAGE_CONSISTENCY_CHECK_JOB_NAME) and passes the payload URL as MULTISTAGE_PARAM_OVERRIDE_PAYLOAD_URL without needing to extract the minor release from it.
Or do you mean something else?
|
|
||
| minor_release = self._get_minor_release_from_payload_url(payload_url) | ||
|
|
||
| job_name = Jobs.STAGE_TESTING_JOB_NAME_TEMPLATE.format(minor_release=minor_release) |
There was a problem hiding this comment.
The job name is already printed on line 427, which is good for consistency with run_image_consistency_check(). Nice work maintaining the same output pattern!
|
/retest |
|
@tomasdavidorg: all tests passed! 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. |
3 participants
https://redhat.atlassian.net/browse/OCPERT-349