fix: correct bundle generation #16
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: SequeI <[email protected]>
Reviewer's GuideThis PR refines the bundle generation script by hardening the shell environment, standardizing tool extraction and path handling, introducing overlay-driven kustomize invocation, and applying post-build CSV adjustments to ensure correct deployment and service naming. Class diagram for script variables and their usageclassDiagram
class build_bundle_sh {
+TOOLS: string
+KUSTOMIZE: string
+IMG: string (optional)
+BUNDLE_OVERLAY: string
+BUNDLE_GEN_FLAGS: string
+CSV: string
+Methods:
+ExtractKustomize()
+DownloadKustomize()
+EditImage()
+BuildManifests()
+GenerateBundle()
+AdjustCSV()
+ValidateBundle()
}
build_bundle_sh --> TOOLS
build_bundle_sh --> KUSTOMIZE
build_bundle_sh --> IMG
build_bundle_sh --> BUNDLE_OVERLAY
build_bundle_sh --> BUNDLE_GEN_FLAGS
build_bundle_sh --> CSV
Flow diagram for the updated bundle generation processflowchart TD
A["Start build-bundle.sh"] --> B["Set TOOLS=/tmp"]
B --> C{Is /cachi2 present?}
C -- Yes --> D["Extract kustomize from /cachi2"]
C -- No --> E["Download kustomize from GitHub"]
D --> F["Set KUSTOMIZE path"]
E --> F
F --> G["Make kustomize executable"]
G --> H["operator-sdk generate kustomize manifests -q"]
H --> I{Is IMG set?}
I -- Yes --> J["Edit image in config/overlays/${BUNDLE_OVERLAY}"]
I -- No --> K["Skip image edit"]
J --> L["Build manifests with kustomize (using overlay)"]
K --> L
L --> M["Pipe manifests to operator-sdk generate bundle"]
M --> N{Is CSV file present?}
N -- Yes --> O["Apply sed replacements for deploymentName and serviceName"]
N -- No --> P["Skip CSV adjustments"]
O --> Q["Remove backup files"]
P --> Q
Q --> R["operator-sdk bundle validate ./bundle"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider parameterizing the kustomize version (e.g. via an environment variable) instead of hard-coding 5.6.0 in multiple places to simplify future upgrades.
- It would be safer to validate or default BUNDLE_OVERLAY at the top of the script to avoid errors when that variable is not set.
- The sed-based CSV edits are brittle—using a kustomize patch or a dedicated overlay to inject the serviceName and adjust deploymentName would be more maintainable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider parameterizing the kustomize version (e.g. via an environment variable) instead of hard-coding 5.6.0 in multiple places to simplify future upgrades.
- It would be safer to validate or default BUNDLE_OVERLAY at the top of the script to avoid errors when that variable is not set.
- The sed-based CSV edits are brittle—using a kustomize patch or a dedicated overlay to inject the serviceName and adjust deploymentName would be more maintainable.
## Individual Comments
### Comment 1
<location> `hack/build-bundle.sh:2` </location>
<code_context>
-#!/bin/bash
-set -e
+#!/usr/bin/env bash
+set -euo pipefail
TOOLS="/tmp"
</code_context>
<issue_to_address>
**issue:** Using 'set -euo pipefail' increases script safety but may break on unset variables.
Ensure all required variables are set or have default values to prevent unexpected script exits due to unset variables.
</issue_to_address>
### Comment 2
<location> `hack/build-bundle.sh:20-22` </location>
<code_context>
+operator-sdk generate kustomize manifests -q
+
+if [[ -n "${IMG:-}" ]]; then
+ pushd "config/overlays/${BUNDLE_OVERLAY}" >/dev/null
+ "${KUSTOMIZE}" edit set image "controller=${IMG}"
+ popd >/dev/null
fi
</code_context>
<issue_to_address>
**issue (bug_risk):** Script assumes BUNDLE_OVERLAY is set and valid, which may cause failures if not.
Validate BUNDLE_OVERLAY before use or set a default to prevent errors if it's unset or invalid.
</issue_to_address>
### Comment 3
<location> `hack/build-bundle.sh:28-33` </location>
<code_context>
+"${KUSTOMIZE}" build "config/overlays/${BUNDLE_OVERLAY}" \
+ | operator-sdk generate bundle ${BUNDLE_GEN_FLAGS}
+
+CSV="bundle/manifests/model-validation-operator.clusterserviceversion.yaml"
+if [[ -f "${CSV}" ]]; then
+ sed -i.bak 's/deploymentName: webhook/deploymentName: model-validation-controller-manager/' "${CSV}"
+ sed -i.bak2 's/deploymentName: model-validation-controller-manager/deploymentName: model-validation-controller-manager\
+ serviceName: model-validation-webhook/' "${CSV}"
+ rm -f "${CSV}.bak" "${CSV}.bak2"
+fi
</code_context>
<issue_to_address>
**suggestion:** Multiple in-place sed operations may be fragile if file contents change.
Consolidating the sed commands or switching to a dedicated YAML processor would improve reliability if the file format changes.
Suggested implementation:
```
CSV="bundle/manifests/model-validation-operator.clusterserviceversion.yaml"
if [[ -f "${CSV}" ]]; then
# Use yq to update deploymentName and add serviceName under the correct path
yq -i '
(.spec.install.spec.deployments[] | select(.name == "model-validation-controller-manager") .spec.template.spec.containers[] | select(.name == "manager") .deploymentName) = "model-validation-controller-manager" |
(.spec.install.spec.deployments[] | select(.name == "model-validation-controller-manager") .spec.template.spec.containers[] | select(.name == "manager") .serviceName) = "model-validation-webhook"
' "${CSV}"
fi
```
- Ensure `yq` is installed and available in your build environment. If not, add installation steps for `yq` before this block.
- The exact YAML path may need adjustment depending on the CSV structure. If the path is different, update the `yq` query accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| #!/bin/bash | ||
| set -e | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail |
There was a problem hiding this comment.
issue: Using 'set -euo pipefail' increases script safety but may break on unset variables.
Ensure all required variables are set or have default values to prevent unexpected script exits due to unset variables.
Comment on lines
+20
to
+22
| "${KUSTOMIZE}" edit set image "controller=${IMG}" | ||
| popd >/dev/null | ||
| fi |
There was a problem hiding this comment.
issue (bug_risk): Script assumes BUNDLE_OVERLAY is set and valid, which may cause failures if not.
Validate BUNDLE_OVERLAY before use or set a default to prevent errors if it's unset or invalid.
Comment on lines
28
to
33
| if [[ -f "${CSV}" ]]; then | ||
| sed -i.bak 's/deploymentName: webhook/deploymentName: model-validation-controller-manager/' "${CSV}" | ||
| sed -i.bak2 's/deploymentName: model-validation-controller-manager/deploymentName: model-validation-controller-manager\ | ||
| serviceName: model-validation-webhook/' "${CSV}" | ||
| rm -f "${CSV}.bak" "${CSV}.bak2" | ||
| fi |
There was a problem hiding this comment.
suggestion: Multiple in-place sed operations may be fragile if file contents change.
Consolidating the sed commands or switching to a dedicated YAML processor would improve reliability if the file format changes.
Suggested implementation:
CSV="bundle/manifests/model-validation-operator.clusterserviceversion.yaml"
if [[ -f "${CSV}" ]]; then
# Use yq to update deploymentName and add serviceName under the correct path
yq -i '
(.spec.install.spec.deployments[] | select(.name == "model-validation-controller-manager") .spec.template.spec.containers[] | select(.name == "manager") .deploymentName) = "model-validation-controller-manager" |
(.spec.install.spec.deployments[] | select(.name == "model-validation-controller-manager") .spec.template.spec.containers[] | select(.name == "manager") .serviceName) = "model-validation-webhook"
' "${CSV}"
fi
- Ensure
yqis installed and available in your build environment. If not, add installation steps foryqbefore this block. - The exact YAML path may need adjustment depending on the CSV structure. If the path is different, update the
yqquery accordingly.
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
JasonPowr
approved these changes
Sep 17, 2025
Signed-off-by: SequeI <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Refine build-bundle.sh to improve robustness, enable dynamic overlay support, and correct generated CSV metadata.
Bug Fixes:
Enhancements: