Make addCRDs() resilient to unparseable manifests, and use shallow clones - #169
Make addCRDs() resilient to unparseable manifests, and use shallow clones#169dislbenn wants to merge 2 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dislbenn The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Warning Review limit reached
Next review available in: 32 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ 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: 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 `@scripts/bundle-generation/bundles-to-charts.py`:
- Around line 2022-2038: Resolve one canonical branch per workflow and reuse it
consistently: in scripts/bundle-generation/bundles-to-charts.py lines 2022-2038,
propagate branch_to_use from the clone step through all downstream version and
resource calls, while keeping default-branch resolution reachable. In
scripts/bundle-generation/generate-charts.py lines 1773-1790, replace the empty
branch value with the actual checked-out default branch. In
scripts/bundle-generation/move-charts.py lines 355-365, replace the synthetic
"main" value with that actual checked-out default branch.
- Around line 1506-1508: Update the addCRDs documentation to accurately describe
all currently propagated failure types: ValueError for bundlePath validation,
OSError from shutil.copyfile failures, and SystemExit from derived bundle lookup
terminating via sys.exit(1); alternatively, normalize these failures to a single
documented exception type.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e2ce1cc-fca7-4a7c-a9e4-0d2b485bc185
📒 Files selected for processing (3)
scripts/bundle-generation/bundles-to-charts.pyscripts/bundle-generation/generate-charts.pyscripts/bundle-generation/move-charts.py
| Raises: | ||
| ValueError: If bundlePath is not found or if CRD file copying fails. | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'Raises:|ValueError|shutil\.copyfile|sys\.exit\(1\)' \
scripts/bundle-generation/bundles-to-charts.py
python - <<'PY'
import shutil
import tempfile
from pathlib import Path
root = Path(tempfile.mkdtemp())
try:
shutil.copyfile(root / "missing", root / "destination")
except Exception as exc:
assert isinstance(exc, OSError), type(exc)
print(type(exc).__name__)
else:
raise SystemExit("Expected copyfile to fail")
PYRepository: stolostron/installer-dev-tools
Length of output: 13747
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1450,1628p' scripts/bundle-generation/bundles-to-charts.py
printf '\naddCRDs call sites:\n'
rg -n -C 4 'addCRDs|getBundleManifestsPath' scripts/bundle-generation/bundles-to-charts.pyRepository: stolostron/installer-dev-tools
Length of output: 11481
Document the actual exceptions raised by addCRDs.
ValueError covers only explicit bundlePath validation. shutil.copyfile propagates OSError, and derived bundle lookup can terminate with SystemExit through sys.exit(1). Document these behaviors or normalize them to one exception type.
🤖 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 `@scripts/bundle-generation/bundles-to-charts.py` around lines 1506 - 1508,
Update the addCRDs documentation to accurately describe all currently propagated
failure types: ValueError for bundlePath validation, OSError from
shutil.copyfile failures, and SystemExit from derived bundle lookup terminating
via sys.exit(1); alternatively, normalize these failures to a single documented
exception type.
| # Check for branch override first, then use config branch | ||
| if repo_name in component_branch_overrides: | ||
| branch_to_use = component_branch_overrides[repo_name] | ||
| logging.info(f"Using branch override for {repo_name}: {branch_to_use}") | ||
| else: | ||
| branch_to_use = branch | ||
|
|
||
| repository.git.checkout(branch_to_use) | ||
| logging.info("Cloning repository: %s from %s (branch=%s)", repo_name, git_url, branch_to_use) | ||
| repo_path = os.path.join(SCRIPT_DIR, "tmp", repo_name) | ||
|
|
||
| if os.path.exists(repo_path): | ||
| shutil.rmtree(repo_path) | ||
|
|
||
| # Shallow, single-branch clone: only the tip commit of the target | ||
| # branch is needed, since this script only reads current file | ||
| # contents (CSVs/CRDs/manifests) and never inspects history. | ||
| Repo.clone_from(git_url, repo_path, branch=branch_to_use, depth=1) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Resolve one canonical branch before cloning and reuse it throughout each workflow.
The clone target can differ from the branch value used by downstream version and resource logic. This can produce charts from one branch while applying rules for another branch.
scripts/bundle-generation/bundles-to-charts.py#L2022-L2038: propagatebranch_to_useto all downstream calls and make default-branch resolution reachable.scripts/bundle-generation/generate-charts.py#L1773-L1790: replace the emptybranchvalue with the actual checked-out default branch.scripts/bundle-generation/move-charts.py#L355-L365: replace the synthetic"main"value with the actual checked-out default branch.
📍 Affects 3 files
scripts/bundle-generation/bundles-to-charts.py#L2022-L2038(this comment)scripts/bundle-generation/generate-charts.py#L1773-L1790scripts/bundle-generation/move-charts.py#L355-L365
🤖 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 `@scripts/bundle-generation/bundles-to-charts.py` around lines 2022 - 2038,
Resolve one canonical branch per workflow and reuse it consistently: in
scripts/bundle-generation/bundles-to-charts.py lines 2022-2038, propagate
branch_to_use from the clone step through all downstream version and resource
calls, while keeping default-branch resolution reachable. In
scripts/bundle-generation/generate-charts.py lines 1773-1790, replace the empty
branch value with the actual checked-out default branch. In
scripts/bundle-generation/move-charts.py lines 355-365, replace the synthetic
"main" value with that actual checked-out default branch.
Mirrors the same change made in stolostron/multiclusterhub-operator (stolostron/multiclusterhub-operator#4579). The three scheduled automation workflows here (regenerate-charts.yml, regenerate-operator-bundles.yml, resync-owner-file.yml) all use peter-evans/create-pull-request with no body input, so every generated PR gets the same generic default text with no information about what changed or whether anything needs a closer look. Add two purpose-built PR templates under .github/PULL_REQUEST_TEMPLATE/ (kept separate from the default human-authored pull_request_template.md, since a bot can't honestly assert things like "I tested this locally"): - automated-bundle-update.md: used by regenerate-charts.yml and regenerate-operator-bundles.yml. Reports the triggering run, a git diff --stat summary, and any manifest files skipped during CRD scanning (tagged UNPARSEABLE_MANIFEST in the log, matching the installer-dev-tools addCRDs() resilience fix in stolostron/installer-dev-tools#169) so a reviewer can verify none of them were meant to be a CRD. - automated-owners-resync.md: used by resync-owner-file.yml, a simpler variant with no warnings section since that workflow has no chart/CRD-generation risk profile. hack/scripts/render_pr_body.py (identical copy of the one added in multiclusterhub-operator) fills in each template's `<!-- AUTOMATION:NAME -->` markers with generated content and writes the result to a file, which is passed to create-pull-request via body-path instead of the default body text. Signed-off-by: dislbenn <dbennett@redhat.com>
Signed-off-by: dislbenn <dbennett@redhat.com>
bundles-to-charts.py, generate-charts.py, and move-charts.py each clone every upstream component repo with a full, unbounded git clone (no --depth limit), then separately check out the target branch locally. None of these scripts read anything beyond the current file contents on that one branch (no git log/blame/diff-against-history usage), so the full commit history was never needed. Switch each clone to a shallow, single-branch clone (depth=1, branch= <target>) resolved up front, and drop the now-redundant separate checkout step. Verified against a component repo with a long release history: .git directory size dropped from 6.2M to 992K for a single clone, and this applies to every component processed on every scheduled run. Signed-off-by: dislbenn <dbennett@redhat.com>
561d8af to
d55ebae
Compare
1 participant
Description
Two related fixes to the bundle-generation scripts: stop the whole tool crashing when a single bundle manifest can't be parsed as plain YAML, and stop doing full (unbounded-history) git clones for every component repo processed.
Related Issue
Found while investigating a failing
stolostron/multiclusterhub-operatorscheduled workflow run (run 31521675972):addCRDs()crashed with an uncaughtyaml.parser.ParserErroron a bundle manifest (networkpolicy.yamlinmulticloud-operators-subscription) that embeds Helm/Go template syntax, which killed chart regeneration for every other component in the same run. Companion shallow-clone changes are also being made instolostron/multiclusterhub-operator(stolostron/multiclusterhub-operator#4579) andstolostron/backplane-operator(stolostron/backplane-operator#3835), which each vendor a copy ofgenerate-shell.pythat clones this repo.Changes Made
addCRDs()resilience (bundles-to-charts.py): wraps the per-file YAML parse intry/except yaml.YAMLError, mirroring the existing pattern already used infind_templates_of_type()for the same kind of directory scan. A file that fails to parse is now logged with anUNPARSEABLE_MANIFEST:prefix (so callers can grep for exactly this condition instead of every routineWARNINGthe script already emits) and skipped, rather than crashing the whole run. This is intentionally non-fatal — it's not added to any error list that would fail the run — since a single malformed file in one upstream repo shouldn't block chart generation for every other component being processed in the same invocation.bundles-to-charts.py,generate-charts.py,move-charts.py): each of these clones every component repo withRepo.clone_from(url, path)(full history, every branch) and then separately checks out the target branch. None of these scripts read anything beyond current file contents on one branch (no git log/blame/diff-against-history usage anywhere in the codebase). Resolved the target branch before cloning and passbranch=/depth=1toclone_from()directly, dropping the now-redundant separate checkout call.Screenshots (if applicable)
N/A
Checklist
Additional Notes
Verified end-to-end against the real failure condition: pointed a scratch
multiclusterhub-operatorcheckout'shack/bundle-automation/config.yamlat a fork branch ofmulticloud-operators-subscriptionstill containing the original, unmodifiednetworkpolicy.yaml(with the Helm template syntax intact), and rangenerate-shell.py --update-charts-from-bundlesagainst this patched branch.yaml.parser.ParserError, exit code 2, entire run fails.UNPARSEABLE_MANIFEST: Skipped 'networkpolicy.yaml' for operator 'multicloud-operators-subscription' while scanning for CRDs — file appears to contain Go/Helm template syntax ('{{ }}'), which is not valid standalone YAML.WARNINGcase-insensitively matches 81 lines (mostly routine, expected output); grepping forUNPARSEABLE_MANIFESTmatches exactly the 1 line that's actually actionable..gitdirectory (full clone) to 992K (shallow, single-branch), with no other code depending on git history for these repos.python3 -m py_compile.Reviewers
/cc @cameronmwall @ngraham20 @gparvin @msmigiel-rh
Definition of Done
Summary by CodeRabbit
Bug Fixes
Improvements