ART-19542: Remove deprecated separate golang branch logic, always use monobranch#3136
Conversation
missing: signing rpm-s
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[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:
WalkthroughThe Golang update pipeline now uses the unified ChangesGolang monobranch and plashet flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant UpdateGolangPipeline
participant Jenkins
participant BuildPlashetsPipeline
participant Doozer
UpdateGolangPipeline->>Jenkins: Start plashet with group golang and VERSION
Jenkins->>BuildPlashetsPipeline: Pass VERSION and OVERRIDE_OCP_VERSION
BuildPlashetsPipeline->>Doozer: Resolve group configuration with MAJOR and MINOR
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✨ 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 |
![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/update_golang.py`:
- Around line 1044-1046: Update the `test_get_doozer_var_args` expectations to
match `_get_doozer_var_args` in `update_golang.py`, which now always returns the
`--var MAJOR=...` and `--var MINOR=...` arguments regardless of
`use_new_golang_branch`. Adjust the test in `test_update_golang.py` so the
default case no longer expects an empty list and both branches assert the
unconditional var output from `UpdateGolangPipeline._get_doozer_var_args`.
🪄 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: f6d02e31-482f-400f-9c62-b891e5ff4285
📒 Files selected for processing (1)
pyartcd/pyartcd/pipelines/update_golang.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyartcd/pyartcd/jenkins.py (1)
776-790: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPass
version=tostart_build_plashets
VERSION=self.ocp_versionnever reaches the newversionparameter, andstart_builddoes not accept arbitrary kwargs, so this path raisesTypeErrorbeforeVERSIONis added to Jenkins params. Switch the call site toversion=self.ocp_version.🤖 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/pyartcd/jenkins.py` around lines 776 - 790, The start_build_plashets call path is not passing the new version argument, so VERSION never makes it into the Jenkins params and the call can fail before that point. Update the call site that invokes start_build_plashets to pass version=self.ocp_version explicitly, and verify the start_build_plashets function’s version parameter is used instead of relying on kwargs from start_build.
🧹 Nitpick comments (2)
pyartcd/pyartcd/plashets.py (2)
406-430: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
build_plashet_from_tagsdocstring not updated for the newversionparameter.Unlike
build_plashets(line 231), the docstring forbuild_plashet_from_tagswasn't updated to document the newversionparam.🤖 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/pyartcd/plashets.py` around lines 406 - 430, The `build_plashet_from_tags` docstring is missing documentation for the new `version` parameter, unlike `build_plashets`. Update the docstring in `build_plashet_from_tags` to include `version` alongside the existing parameters, keeping the description consistent with the function signature and related plashet builder documentation.
439-446: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnguarded
version.split('.')unpack can crash on malformed input.If
versiondoesn't split into exactly two dot-separated parts (e.g. an accidental4.18.2or trailing garbage from CLI/Jenkins input),major, minor = version.split('.')raises an unhandledValueError, aborting the plashet build. This mirrors the same fragile pattern already used in_get_doozer_var_args(update_golang.py), so consider centralizing MAJOR/MINOR parsing (e.g. a small sharedparse_major_minor(version)helper with a clear error message) instead of duplicating the unguarded split logic across files.🛡️ Proposed guard
if version: - major, minor = version.split('.') + parts = version.split('.') + if len(parts) != 2: + raise ValueError(f"Expected version in MAJOR.MINOR format, got: {version!r}") + major, minor = parts cmd.extend(['--var', f'MAJOR={major}', '--var', f'MINOR={minor}'])🤖 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/pyartcd/plashets.py` around lines 439 - 446, The MAJOR/MINOR extraction in plashets command assembly is using an unguarded version.split('.') unpack that can raise ValueError on malformed input. Update the logic around the version handling in plashets.py to validate the version format before unpacking, and reuse a shared helper such as parse_major_minor(version) if available or create one so the parsing and error message are consistent with _get_doozer_var_args in update_golang.py. Ensure the helper returns or raises a clear, user-facing error when the version is not exactly major.minor.
🤖 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.
Outside diff comments:
In `@pyartcd/pyartcd/jenkins.py`:
- Around line 776-790: The start_build_plashets call path is not passing the new
version argument, so VERSION never makes it into the Jenkins params and the call
can fail before that point. Update the call site that invokes
start_build_plashets to pass version=self.ocp_version explicitly, and verify the
start_build_plashets function’s version parameter is used instead of relying on
kwargs from start_build.
---
Nitpick comments:
In `@pyartcd/pyartcd/plashets.py`:
- Around line 406-430: The `build_plashet_from_tags` docstring is missing
documentation for the new `version` parameter, unlike `build_plashets`. Update
the docstring in `build_plashet_from_tags` to include `version` alongside the
existing parameters, keeping the description consistent with the function
signature and related plashet builder documentation.
- Around line 439-446: The MAJOR/MINOR extraction in plashets command assembly
is using an unguarded version.split('.') unpack that can raise ValueError on
malformed input. Update the logic around the version handling in plashets.py to
validate the version format before unpacking, and reuse a shared helper such as
parse_major_minor(version) if available or create one so the parsing and error
message are consistent with _get_doozer_var_args in update_golang.py. Ensure the
helper returns or raises a clear, user-facing error when the version is not
exactly major.minor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: da678086-e0db-4c3f-b17a-e9481182fea8
📒 Files selected for processing (5)
pyartcd/pyartcd/jenkins.pypyartcd/pyartcd/pipelines/build_plashets.pypyartcd/pyartcd/pipelines/update_golang.pypyartcd/pyartcd/plashets.pypyartcd/tests/pipelines/test_update_golang.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pyartcd/pyartcd/pipelines/update_golang.py
kopero2000
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@kopero2000: This pull request references ART-19542 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 ticket to target the "5.0.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. |
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/jenkins.py`:
- Around line 788-789: Only include the VERSION entry in the parameter mapping
built by the relevant Jenkins helper when version is not None; preserve
OVERRIDE_OCP_VERSION and existing behavior for callers that omit version, so
urlencode() never emits VERSION=None.
🪄 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: ed97b025-30f2-432e-9443-048b1b99294f
📒 Files selected for processing (5)
pyartcd/pyartcd/jenkins.pypyartcd/pyartcd/pipelines/build_plashets.pypyartcd/pyartcd/pipelines/update_golang.pypyartcd/pyartcd/plashets.pypyartcd/tests/pipelines/test_update_golang.py
🚧 Files skipped from review as they are similar to previous changes (4)
- pyartcd/tests/pipelines/test_update_golang.py
- pyartcd/pyartcd/plashets.py
- pyartcd/pyartcd/pipelines/update_golang.py
- pyartcd/pyartcd/pipelines/build_plashets.py
| 'OVERRIDE_OCP_VERSION': override_ocp_version, | ||
| 'VERSION': version |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all callers of start_build_plashets to verify whether version is always provided.
rg -n -A5 'start_build_plashets' --type py pyartcd/Repository: openshift-eng/art-tools
Length of output: 12023
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== start_build_plashets definition =="
sed -n '776,805p' pyartcd/pyartcd/jenkins.py
echo
echo "== build_microshift_bootc caller =="
sed -n '500,530p' pyartcd/pyartcd/pipelines/build_microshift_bootc.py
echo
echo "== scan_plashet_rpms caller =="
sed -n '295,320p' pyartcd/pyartcd/pipelines/scan_plashet_rpms.py
echo
echo "== ocp caller =="
sed -n '790,812p' pyartcd/pyartcd/pipelines/ocp.py
echo
echo "== ocp4_konflux caller =="
sed -n '1010,1030p' pyartcd/pyartcd/pipelines/ocp4_konflux.py
echo
echo "== update_golang caller =="
sed -n '612,632p' pyartcd/pyartcd/pipelines/update_golang.pyRepository: openshift-eng/art-tools
Length of output: 6187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== start_build implementation =="
sed -n '330,375p' pyartcd/pyartcd/jenkins.py
echo
echo "== Jenkins client usage/search =="
rg -n "build_params|invoke\\(" pyartcd/pyartcd -g '*.py'
echo
echo "== Any explicit handling of None params =="
rg -n "None.*VERSION|VERSION.*None|bool.*build_params|clean.*params" pyartcd/pyartcd -g '*.py'Repository: openshift-eng/art-tools
Length of output: 1915
🌐 Web query:
python-jenkins build_params None invoke build parameter serialization
💡 Result:
In the python-jenkins library, the build_job(name, parameters=None, token=None) method handles parameter serialization by converting the provided dictionary or list of tuples into a URL-encoded string [1][2]. When you invoke build_job with the parameters argument, the library internally calls build_job_url, which performs the following logic [2]: 1. Serialization: It uses urllib.urlencode to convert the parameters dictionary (or list of two-membered tuples) into a standard URL-encoded query string [2]. 2. Endpoint Selection: - If parameters are provided (i.e., parameters is not None), the library targets the buildWithParameters endpoint [2]. - If parameters is None, it targets the standard build endpoint [2]. 3. Execution: The resulting URL (including the query string if parameters are present) is requested using the internal jenkins_open method [2]. Key technical considerations: - Data Structure: The parameters argument must be a dict or a list of two-membered tuples [1][2]. Using a list of tuples is specifically recommended when you need to supply multiple values for parameters with multi-select options [1]. - Job Configuration: The Jenkins job itself must be configured to accept parameters [3][4]. If you pass parameters to a job that is not configured to accept them, Jenkins will return an error (typically a 500 status code or a javax.servlet.ServletException) [3][4]. - API Behavior: Because build_job relies on URL serialization, ensure that your parameter values are compatible with URL encoding [2]. If a job requires complex data or large payloads, this standard URL-based triggering mechanism may be insufficient, as it is designed for simple key-value parameter pairs [2].
Citations:
- 1: https://python-jenkins.readthedocs.io/en/stable/api.html
- 2: https://github.com/jjh42/python-jenkins/blob/master/jenkins/__init__.py
- 3: https://bugs.launchpad.net/bugs/1177831
- 4: https://stackoverflow.com/questions/43155042/500-server-error-when-using-jenkins-python-module-build-jobjob-name-parameters
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from urllib.parse import urlencode
cases = [
{"VERSION": None},
{"OVERRIDE_OCP_VERSION": False},
{"OVERRIDE_OCP_VERSION": True},
{"VERSION": "4.18"},
]
for case in cases:
print(case, "->", urlencode(case))
PYRepository: openshift-eng/art-tools
Length of output: 353
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "build-plashets|BUILD_PLASHETS|VERSION" -g '*.yml' -g '*.yaml' -g '*.groovy' -g '*.jenkins*' -g '*.xml' -g '*.py' pyartcd .claude .Repository: openshift-eng/art-tools
Length of output: 1969
Only add VERSION when a version is set. In pyartcd/pyartcd/jenkins.py:788-789, urlencode() turns None into the literal VERSION=None, so callers that omit version now send a bogus parameter instead of omitting it. That changes the previous behavior and can break plashet builds.
🤖 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/pyartcd/jenkins.py` around lines 788 - 789, Only include the VERSION
entry in the parameter mapping built by the relevant Jenkins helper when version
is not None; preserve OVERRIDE_OCP_VERSION and existing behavior for callers
that omit version, so urlencode() never emits VERSION=None.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/plashets.py`:
- Around line 259-260: Validate the format of version in the
override_ocp_version handling before unpacking it: split on “.” and require
exactly two non-empty components, otherwise raise a clear ValueError describing
the expected major.minor format. Update the logic around the existing version
processing in plashets.py so valid values still assign major and minor safely.
- Around line 259-261: The MAJOR extra variable in the override OCP version
handling incorrectly includes a literal dollar sign. Update the version parsing
block to construct MAJOR from the parsed major value without "$", matching the
existing MINOR assignment and ensuring doozer receives the numeric version.
In `@pyartcd/pyartcd/util.py`:
- Line 85: Replace the mutable default for extra_vars with a None default and
annotate it as List[str] | None in the affected function; initialize it to an
empty list inside the function before use, preserving existing behavior and
satisfying Ruff B006.
- Around line 92-94: Reorder the argument construction in the affected utility
command so the `group` value immediately follows `"--group"`, then append each
`--var` and its corresponding `extra_vars` value. Update the list expression
around the command-building logic to ensure `doozer` parses the group correctly
when extra variables are present.
🪄 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: 7c427cf9-93a8-460f-93d6-35c393231da9
📒 Files selected for processing (2)
pyartcd/pyartcd/plashets.pypyartcd/pyartcd/util.py
| if(override_ocp_version and version): | ||
| major, minor = version.split('.') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
version.split('.') will crash on versions with ≠2 components.
major, minor = version.split('.') raises ValueError if version has more or fewer than 2 dot-separated parts (e.g., "4.18.0" → too many values to unpack; "4" → not enough values to unpack). The docstring says version is e.g. '4.18', but there's no validation. A caller passing a three-part version would get an opaque unpacking error.
🛡️ Proposed fix — validate version format
if(override_ocp_version and version):
- major, minor = version.split('.')
+ parts = version.split('.')
+ if len(parts) != 2:
+ raise ValueError(f"version must be in X.Y format, got: {version}")
+ major, minor = parts
extra_vars = [f"MAJOR={major}", f"MINOR={minor}"]📝 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.
| if(override_ocp_version and version): | |
| major, minor = version.split('.') | |
| if(override_ocp_version and version): | |
| parts = version.split('.') | |
| if len(parts) != 2: | |
| raise ValueError(f"version must be in X.Y format, got: {version}") | |
| major, minor = parts | |
| extra_vars = [f"MAJOR={major}", f"MINOR={minor}"] |
🤖 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/pyartcd/plashets.py` around lines 259 - 260, Validate the format of
version in the override_ocp_version handling before unpacking it: split on “.”
and require exactly two non-empty components, otherwise raise a clear ValueError
describing the expected major.minor format. Update the logic around the existing
version processing in plashets.py so valid values still assign major and minor
safely.
| if(override_ocp_version and version): | ||
| major, minor = version.split('.') | ||
| extra_vars = [f"MAJOR=${major}", f"MINOR={minor}"] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether build_plashet_from_tags (lines ~448-456) has the same $ bug
sed -n '430,470p' pyartcd/pyartcd/plashets.pyRepository: openshift-eng/art-tools
Length of output: 1333
Remove the literal $ from the MAJOR f-string. f"MAJOR=${major}" passes MAJOR=$4 to doozer; it should be MAJOR=4, otherwise version-based plashet path construction breaks when override_ocp_version is set.
🤖 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/pyartcd/plashets.py` around lines 259 - 261, The MAJOR extra variable
in the override OCP version handling incorrectly includes a literal dollar sign.
Update the version parsing block to construct MAJOR from the parsed major value
without "$", matching the existing MINOR assignment and ensuring doozer receives
the numeric version.
| env=None, | ||
| doozer_data_path: str = constants.OCP_BUILD_DATA_URL, | ||
| doozer_data_gitref: str = '', | ||
| extra_vars: List=[] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Mutable default argument extra_vars: List=[] — use None instead.
Ruff B006 flags this. Although the function only reads extra_vars (no mutation), mutable defaults are a well-known Python anti-pattern. Since the project targets Python 3.11, List[str] | None is valid syntax.
♻️ Proposed fix — use None default with in-function initialization
- extra_vars: List=[]
+ extra_vars: List[str] | None = None
) -> Dict:
+ if extra_vars is None:
+ extra_vars = []
if doozer_data_gitref:📝 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.
| extra_vars: List=[] | |
| extra_vars: List[str] | None = None | |
| ) -> Dict: | |
| if extra_vars is None: | |
| extra_vars = [] | |
| if doozer_data_gitref: |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 85-85: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🤖 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/pyartcd/util.py` at line 85, Replace the mutable default for
extra_vars with a None default and annotate it as List[str] | None in the
affected function; initialize it to an empty list inside the function before
use, preserving existing behavior and satisfying Ruff B006.
Source: Linters/SAST tools
| "--group", | ||
| *[param for extra_var in extra_vars for param in ['--var', extra_var]], | ||
| group, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify doozer's --group argument parser definition (nargs vs standard store)
rg -n "add_argument.*--group" doozer/ --type=py -C2
# Also check --var definition
rg -n "add_argument.*--var" doozer/ --type=py -C2Repository: openshift-eng/art-tools
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- util.py around reported lines ---'
sed -n '1,180p' pyartcd/pyartcd/util.py | cat -n
echo '--- search load_group_config usage/tests ---'
rg -n "load_group_config|config:read-group|--group|--var" pyartcd/ -g '*.py' -C 2Repository: openshift-eng/art-tools
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- doozer --group / --var definitions ---'
rg -n '`@click`\.option\(|add_argument\(' doozer -g '*.py' -C 2 | rg -n --context 0 --no-heading '--group|--var|config:read-group|context_settings|allow_interspersed_args'
echo '--- doozer command entrypoints mentioning config:read-group ---'
rg -n 'config:read-group|read-group' doozer -g '*.py' -C 3Repository: openshift-eng/art-tools
Length of output: 679
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- locate doozer CLI entrypoint files ---'
git ls-files doozer | rg '(^|/)cli/|__main__\.py$|main\.py$|click|argparse'
echo '--- search for click group/parser settings ---'
rg -n --fixed-strings 'allow_interspersed_args' doozer -g '*.py' -C 2 || true
rg -n --fixed-strings 'ignore_unknown_options' doozer -g '*.py' -C 2 || true
echo '--- search for --var and config:read-group definitions ---'
rg -n -- '--var|config:read-group|click\.group|click\.option\(\s*[\'"]--group|click\.option\(\s*[\'"]--var' doozer -g '*.py' -C 3 || trueRepository: openshift-eng/art-tools
Length of output: 3087
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- AST outline of doozer CLI entrypoints ---'
ast-grep outline doozer/doozerlib/cli/__init__.py --view expanded || true
ast-grep outline doozer/doozerlib/cli/__main__.py --view expanded || true
ast-grep outline doozer/doozerlib/cli/cli_opts.py --view expanded || true
ast-grep outline doozer/doozerlib/cli/config.py --view expanded || true
echo '--- search for var/group/config:read-group with safe quoting ---'
rg -n --fixed-strings '--var' doozer/doozerlib/cli -g '*.py' -C 3 || true
rg -n --fixed-strings 'config:read-group' doozer/doozerlib/cli -g '*.py' -C 3 || true
rg -n --fixed-strings '--group' doozer/doozerlib/cli/cli_opts.py doozer/doozerlib/cli/config.py -g '*.py' -C 3 || trueRepository: openshift-eng/art-tools
Length of output: 5515
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- root CLI options around group setup ---'
sed -n '200,280p' doozer/doozerlib/cli/__init__.py | cat -n
echo '--- cli options defaults/definitions ---'
sed -n '1,120p' doozer/doozerlib/cli/cli_opts.py | cat -n
echo '--- exact --var and --group option definitions in doozer CLI ---'
rg -n --fixed-strings -- '--var' doozer/doozerlib/cli -g '*.py' -C 3 || true
rg -n --fixed-strings -- '--group' doozer/doozerlib/cli/__init__.py doozer/doozerlib/cli/cli_opts.py -C 3 || true
python3 - <<'PY'
import click
`@click.group`(context_settings=dict(help_option_names=['-h', '--help']))
`@click.option`('--group')
`@click.option`('--var', multiple=True)
def cli(group, var):
click.echo(f'group={group!r} var={var!r}')
`@cli.command`()
def config():
pass
# Exercise argv shapes relevant to the review comment.
for argv in [
['--group', '--var', 'MAJOR=4', '--var', 'MINOR=18', 'golang', 'config'],
['--group', 'golang', '--var', 'MAJOR=4', '--var', 'MINOR=18', 'config'],
['--group=golang', '--var', 'MAJOR=4', '--var', 'MINOR=18', 'config'],
]:
try:
rv = cli.main(args=argv, prog_name='doozer', standalone_mode=False)
print('argv', argv, '-> ok')
except Exception as e:
print('argv', argv, '->', type(e).__name__, e)
PYRepository: openshift-eng/art-tools
Length of output: 7399
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- root CLI options around group setup ---'
sed -n '200,280p' doozer/doozerlib/cli/__init__.py | cat -n
echo '--- cli options defaults/definitions ---'
sed -n '1,120p' doozer/doozerlib/cli/cli_opts.py | cat -n
echo '--- exact --var and --group option definitions in doozer CLI ---'
rg -n --fixed-strings -- '--var' doozer/doozerlib/cli -g '*.py' -C 3 || true
rg -n --fixed-strings -- '--group' doozer/doozerlib/cli/__init__.py doozer/doozerlib/cli/cli_opts.py -C 3 || true
python3 - <<'PY'
import click
`@click.group`(context_settings=dict(help_option_names=['-h', '--help']))
`@click.option`('--group')
`@click.option`('--var', multiple=True)
def cli(group, var):
click.echo(f'group={group!r} var={var!r}')
`@cli.command`()
def config():
pass
for argv in [
['--group', '--var', 'MAJOR=4', '--var', 'MINOR=18', 'golang', 'config'],
['--group', 'golang', '--var', 'MAJOR=4', '--var', 'MINOR=18', 'config'],
['--group=golang', '--var', 'MAJOR=4', '--var', 'MINOR=18', 'config'],
]:
try:
cli.main(args=argv, prog_name='doozer', standalone_mode=False)
print('argv', argv, '-> ok')
except Exception as e:
print('argv', argv, '->', type(e).__name__, e)
PYRepository: openshift-eng/art-tools
Length of output: 7399
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- callers passing extra_vars to load_group_config ---'
rg -n --fixed-strings 'extra_vars=' pyartcd doozer -g '*.py' -C 2 || true
echo '--- direct load_group_config calls ---'
rg -n --fixed-strings 'load_group_config(' pyartcd -g '*.py' -C 2 || trueRepository: openshift-eng/art-tools
Length of output: 10593
Move --var after the --group value. doozer reads --group’s value immediately, so this order shifts --var into the group slot and breaks the command whenever extra_vars is set.
🤖 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/pyartcd/util.py` around lines 92 - 94, Reorder the argument
construction in the affected utility command so the `group` value immediately
follows `"--group"`, then append each `--var` and its corresponding `extra_vars`
value. Update the list expression around the command-building logic to ensure
`doozer` parses the group correctly when extra variables are present.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/jenkins.py`:
- Around line 788-791: Remove the hardcoded developer branch value from
ART_TOOLS_COMMIT, or source it from an appropriate configurable or dynamically
determined value. Update the parameter construction in the relevant Jenkins
build function to include VERSION only when version is not None, preventing
urlencode() from emitting VERSION=None.
- Line 48: Change the BUILD_PLASHETS constant back to the shared Jenkins job
identifier aos-cd-builds/build%2Fbuild-plashets instead of the personal
hack/bvizi namespace.
🪄 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: ff668231-8cb8-41c8-919f-c67c95756f82
📒 Files selected for processing (1)
pyartcd/pyartcd/jenkins.py
| CINCINNATI_PRS = 'aos-cd-builds/build%2Fcincinnati-prs' | ||
| RHCOS_SYNC = 'aos-cd-builds/build%2Frhcos_sync' | ||
| BUILD_PLASHETS = 'aos-cd-builds/build%2Fbuild-plashets' | ||
| BUILD_PLASHETS = 'hack/bvizi/build%2Fbuild-plashets' |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Jenkins helper and nearby context
sed -n '1,120p' pyartcd/pyartcd/jenkins.py
# Find all references to the job constant and related plashet build logic
rg -n "BUILD_PLASHETS|build-plashets|hack/bvizi|aos-cd-builds" pyartcd -S
# Show the current diff for the file, limited to the relevant hunks
git diff -- pyartcd/pyartcd/jenkins.pyRepository: openshift-eng/art-tools
Length of output: 9788
Change BUILD_PLASHETS back to the shared Jenkins job. hack/bvizi/build%2Fbuild-plashets is a personal namespace and will route plashet triggers to the wrong job; this should stay under aos-cd-builds/build%2Fbuild-plashets.
🤖 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/pyartcd/jenkins.py` at line 48, Change the BUILD_PLASHETS constant
back to the shared Jenkins job identifier aos-cd-builds/build%2Fbuild-plashets
instead of the personal hack/bvizi namespace.
| 'OVERRIDE_OCP_VERSION': override_ocp_version, | ||
| 'VERSION': version, | ||
| 'ART_TOOLS_COMMIT': "kopero2000@cleanup-logic-for-seperate-golang-versions", | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Remove hardcoded developer branch reference and conditionally include VERSION.
Two issues in this segment:
-
ART_TOOLS_COMMITis hardcoded to"kopero2000@cleanup-logic-for-seperate-golang-versions"— this forces all plashet builds to use a specific developer's uncommitted branch. This looks like a development artifact that was accidentally left in. It should either be removed, made configurable, or set dynamically. -
VERSIONis unconditionally included even whenversionisNone—urlencode()serializesNoneas the literal string"None", sendingVERSION=Noneto Jenkins. Callers that omitversion(e.g., non-golang plashet builds) will now send a bogus parameter that can break builds. This was flagged in a previous review and remains unaddressed.
🔧 Proposed fix
'DRY_RUN': dry_run,
'OVERRIDE_OCP_VERSION': override_ocp_version,
- 'VERSION': version,
- 'ART_TOOLS_COMMIT': "kopero2000@cleanup-logic-for-seperate-golang-versions",
+ 'ART_TOOLS_COMMIT': art_tools_commit, # or remove entirely
}
+if version is not None:
+ params['VERSION'] = version🤖 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/pyartcd/jenkins.py` around lines 788 - 791, Remove the hardcoded
developer branch value from ART_TOOLS_COMMIT, or source it from an appropriate
configurable or dynamically determined value. Update the parameter construction
in the relevant Jenkins build function to include VERSION only when version is
not None, preventing urlencode() from emitting VERSION=None.
2 participants
Summary
This PR removes the deprecated separate golang branch logic from the
update-golangpipeline and makes it always use the unifiedgolangmonobranch layout. Additionally, it implements support for golang RPM signing and plashet workflow with the monobranch (ART-21532).Changes
Update Golang Pipeline (
update_golang.py)--use-new-golang-branchflag is not set (default), a warning is logged directing users to the golang monobranchget_golang_branch()method: No longer needed since we always use'golang'monobranch_get_doozer_var_args(): Always returns MAJOR/MINOR OCP version variables (needed for monobranch template substitution)_get_doozer_group_and_image(): Always usesGOLANG_DATA_BRANCH = 'golang'and versioned image keysverify_golang_builder_repo(): Always checks the'golang'monobranchshould_sign_golang_rpm(): Always reads from'golang'monobranch group.yml_build_golang_plashets(): Now uses monobranch and passes OCP version for MAJOR/MINOR variable resolution--use-new-golang-branchas deprecatedPlashet Support for Monobranch (ART-21532 fix)
plashets.py: Addedversionparameter tobuild_plashets()andbuild_plashet_from_tags()functionsbuild_plashets.py: Added--versionCLI option and passed through to BuildPlashetsPipelinejenkins.py: AddedVERSIONparameter support tostart_build_plashets()functionversionis provided, the plashet build now injects--var MAJOR=X --var MINOR=Ybefore the--groupargument, allowing doozer to resolve template placeholders in the monobranch group.ymlTest Updates
test_get_golang_branch(method no longer exists)test_get_doozer_var_argsto expect vars are always returnedtest_rebase_brewto expect monobranch group name and --var argumentstest_returns_true_when_config_setand related tests to expectref='golang'instead of separate branch namestest_triggers_jenkins_for_each_el_versionto verify VERSION parameter is passedBackward Compatibility
The
--use-new-golang-branchparameter is kept to:The flag is now effectively ignored - the pipeline always uses the monobranch approach.
Unused
--vararguments passed to doozer are harmless - they're simply ignored if the group.yml doesn't have corresponding template placeholders, making this change safe for both old and new branch layouts.Testing
82 passed)Related Issues
Summary by CodeRabbit
--versionand can override the default OCP version to derive major/minor values for configuration.golanglayout for more reliable version/template resolution.--use-new-golang-branchoption is deprecated: a warning is emitted and the flag is ignored.