fix(ci): as to not drop the comments of the templated yaml#1948
Conversation
cwrau
requested review from
SyeKlu and
Copilot
and removed request for
a team
Summary of ChangesHello @cwrau, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the CI scripts responsible for processing and organizing YAML configuration files, particularly those generated from HelmReleases. The primary motivation is to ensure that comments and original formatting are preserved when these YAML documents are split and templated, addressing a potential issue where such information might have been inadvertently dropped. The changes introduce a more robust and modular approach to handling multi-document YAML files within the CI pipeline. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughRefactors YAML-splitting to use csplit + parallel with new exported helpers, updates path resolution in extract-artifacthub-images.sh to use Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow
participant SplitScript as splitYamlIntoDir
participant Csplit
participant Parallel
participant Rename as renameFile
participant SplitHelm as splitHelmReleaseIntoDir
participant Template as templateHelmRelease
Workflow->>SplitScript: call with YAML input
SplitScript->>Csplit: split input into per-doc files
Csplit-->>SplitScript: list of split files
SplitScript->>Parallel: process split files concurrently
Parallel->>Rename: read metadata and move file to dir/namespace/kind/name.yaml
Parallel->>SplitHelm: detect HelmRelease -> request templating
SplitHelm->>Template: template HelmRelease via BIN_DIR/templateHelmRelease
Template-->>SplitHelm: templated YAML
SplitHelm-->>Parallel: return processed documents
Parallel-->>SplitScript: all files processed
SplitScript-->>Workflow: completion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the YAML splitting logic to use csplit instead of yq. This is a great improvement as it preserves comments from the original files, which was the main goal. The new script is well-structured into functions and leverages parallel for better performance. I've found a couple of areas where the script's robustness can be improved to prevent potential failures. My comments focus on handling empty YAML documents gracefully and ensuring environment variables are correctly scoped for all execution paths, including recursive function calls.
There was a problem hiding this comment.
Pull request overview
Updates the CI/scripts used to template and split Helm-generated YAML so that comments in templated manifests are preserved, and adjusts related CI checks and helper scripts.
Changes:
- Remove several chart validation steps from the
linterGitHub Actions workflow. - Rewrite
.github/scripts/splitYamlIntoDirto split YAML usingcsplitand process outputs in parallel (instead of re-serializing viayq). - Fix
extract-artifacthub-images.shto correctly build theexistingValuesDirpath from theexistingDirvariable.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| .github/workflows/linter.yaml | Removes artifacthub/registry/license checks from the lint workflow job. |
| .github/scripts/splitYamlIntoDir | Replaces YAML splitting logic with csplit + parallel and adds helper functions for renaming/splitting HelmRelease outputs. |
| .github/scripts/extract-artifacthub-images.sh | Corrects path construction for locating existing artifacthub-values directories. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🤖 Fix all issues with AI agents
In @.github/scripts/splitYamlIntoDir:
- Around line 42-49: The recursion builds a duplicated path because
helmreleaseDir already contains the parent dirname; in splitYamlIntoDir you
should pass the already-built helmreleaseDir as the target instead of prepending
dirname again. Update the recursive call to use helmreleaseDir (the variable
created by helmreleaseDir="$(dirname "$helmrelease")/$(basename -s .yaml
"$helmrelease")") so splitYamlIntoDir is invoked with "$helmreleaseDir" rather
than "$(dirname "$helmrelease")/$helmreleaseDir".
- Around line 13-26: Normalize yq nulls and validate required metadata: change
the yq invocations that set kind, namespace, and name to use the null-coalescing
form (e.g. yq -r '.kind // ""' and yq -r '.metadata.namespace // ""', and yq -r
'.metadata.name // ""') so literal "null" becomes an empty string; then update
the early-return check (the block using kind) to treat both empty and "null" as
empty by relying on the normalized value, and add a validation after reading
name to error/return if name is empty (prevent constructing paths with missing
names) referencing the variables kind, namespace, name and the splitFile
handling logic.
🧹 Nitpick comments (1)
.github/scripts/splitYamlIntoDir (1)
58-69: Makecsplitrobust for relative YAML paths.Because you
env -C "$yamlDir", passing a relative$yaml(e.g.,foo/bar.yaml) will resolve tofoo/foo/bar.yaml. Usingbasenamekeeps both absolute and relative callers safe.♻️ Suggested tweak
- env -C "$yamlDir" csplit --prefix="$splitPrefix" --suppress-matched "$yaml" '/^---$/' '{*}' >/dev/null + env -C "$yamlDir" csplit --prefix="$splitPrefix" --suppress-matched "$(basename "$yaml")" '/^---$/' '{*}' >/dev/null
cwrau
force-pushed
the
fix/ci/restore-image-skipping
branch
from
8fc31e5 to
692a907
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/scripts/splitYamlIntoDir:
- Around line 54-69: The csplit call in splitYamlIntoDir aborts the script under
set -e for single-document YAML because csplit returns 1 when no separator is
found; change the call to capture its exit code and only treat codes other than
0 or 1 as fatal. Concretely, run env -C "$yamlDir" csplit ... >/dev/null, save
its exit code (e.g., rc=$?), and if rc is not 0 and not 1 then propagate/exit
with that rc; otherwise continue normally so single-document YAML is accepted.
Ensure this logic is applied around the csplit invocation in splitYamlIntoDir.
🧹 Nitpick comments (1)
.github/scripts/splitYamlIntoDir (1)
70-74: Confirm GNUparallelavailability or add a guard.
Ifparallelisn’t installed on all runners, this will fail underset -e. Consider a preflight check (or fallback).🛠️ Minimal guard example
+ command -v parallel >/dev/null || { echo "GNU parallel is required" >&2; return 1; } find "$yamlDir" -type f -name "${splitPrefix}*" -print0 | parallel -0 renameFile {} "$dir" find "$dir" -type d -name HelmRelease -exec find {} -maxdepth 1 -type f -name '*.yaml' -print0 \; | parallel -0 splitHelmReleaseIntoDir {} "$yaml"
cwrau
force-pushed
the
fix/ci/restore-image-skipping
branch
from
692a907 to
2848437
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/scripts/splitYamlIntoDir:
- Around line 27-34: The current check-then-move sequence using resourceName
(the variable and the mv step inside the block) is vulnerable to a TOCTOU race
when renameFile is called in parallel; change the logic to avoid the pre-check
and perform an atomic move with mv -n (or equivalent) to fail if the destination
exists, and detect that failure to return non-zero and log the "'$resourceName'
shouldn't already exist" message; ensure mkdir -p "$(dirname "$resourceName")"
still runs before the mv, and remove the separate -f existence test so
duplicates are deterministically rejected by the mv command.
2848437 to
b911ab3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/scripts/splitYamlIntoDir:
- Around line 38-52: The function splitHelmReleaseIntoDir calls
"$BIN_DIR/templateHelmRelease" but BIN_DIR is only assigned in the parent script
without being exported, so subshells (e.g., spawned by parallel) won't see it;
fix by exporting BIN_DIR where it's defined (or export it immediately before
invoking parallel) so splitHelmReleaseIntoDir and any subshells can access
BIN_DIR; ensure you add export BIN_DIR when you set its value (or run export
BIN_DIR="$BIN_DIR" prior to the parallel invocation) so the templateHelmRelease
call inside splitHelmReleaseIntoDir works in subshells.
🧹 Nitpick comments (1)
.github/scripts/splitYamlIntoDir (1)
74-76: Consider adding--haltoption toparallelfor fail-fast behavior.If any
renameFileorsplitHelmReleaseIntoDircall fails,parallelcontinues processing remaining items by default. With strict mode (set -e), the script should fail early on errors.Suggested improvement
- find "$yamlDir" -type f -name "${splitPrefix}*" -print0 | parallel -0 renameFile {} "$dir" + find "$yamlDir" -type f -name "${splitPrefix}*" -print0 | parallel -0 --halt now,fail=1 renameFile {} "$dir" - find "$dir" -type d -name HelmRelease -exec find {} -maxdepth 1 -type f -name '*.yaml' -print0 \; | parallel -0 splitHelmReleaseIntoDir {} "$yaml" + find "$dir" -type d -name HelmRelease -exec find {} -maxdepth 1 -type f -name '*.yaml' -print0 \; | parallel -0 --halt now,fail=1 splitHelmReleaseIntoDir {} "$yaml"
Summary by CodeRabbit
Refactor
New Features
Bug Fixes
Chores