fix: avoid double helm upgrade in CI install script - #174
Merged
openshift-merge-bot[bot] merged 5 commits intoJun 26, 2025
Conversation
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
rm3l
reviewed
Jun 23, 2025
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
rm3l
reviewed
Jun 24, 2025
Member
|
@sourcery-ai review |
Reviewer's GuideThe CI install script’s helm deployment has been streamlined by removing the redundant initial upgrade, consolidating parameter collection (including clusterRouterBase) and PostgreSQL password handling into a single helm upgrade invocation, and adding logic to preserve or generate the database password before deployment. Sequence diagram for streamlined Helm upgrade in CI install scriptsequenceDiagram
actor CI_User as CI/CD Pipeline
participant Script as install.sh
participant K8s as Kubernetes Cluster
participant Helm as Helm
participant Secret as PostgreSQL Secret
CI_User->>Script: Run install.sh
Script->>K8s: Check for redhat-developer-hub-postgresql secret
alt Secret exists
Script->>Secret: Retrieve password
else Secret does not exist
Script->>Script: Generate new password
end
Script->>K8s: Get clusterRouterBase
Script->>Helm: helm upgrade redhat-developer-hub ... --set clusterRouterBase --set postgresql.password
Helm->>K8s: Deploy/Upgrade application (single rollout)
K8s-->>CI_User: Deployment complete
Flow diagram for improved password handling in install scriptflowchart TD
A[Start install.sh] --> B{Does PostgreSQL secret exist?}
B -- Yes --> C[Retrieve password from secret]
B -- No --> D[Generate new password]
C --> E[Collect clusterRouterBase]
D --> E
E --> F[Run helm upgrade with collected values]
F --> G[Deployment complete]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @Fortune-Ndlovu - I've reviewed your changes - here's some feedback:
- Add
set -euo pipefailat the top of the script to ensure it fails fast on errors and unset variables. - Consider adding the
--waitflag to thehelm upgradeinvocation so the script waits for a successful rollout before exiting. - Standardize on either
ocorkubectlthroughout the script to avoid requiring both CLIs in your CI environment.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Add `set -euo pipefail` at the top of the script to ensure it fails fast on errors and unset variables.
- Consider adding the `--wait` flag to the `helm upgrade` invocation so the script waits for a successful rollout before exiting.
- Standardize on either `oc` or `kubectl` throughout the script to avoid requiring both CLIs in your CI environment.
## Individual Comments
### Comment 1
<location> `.rhdh/scripts/install.sh:138` </location>
<code_context>
-helm upgrade redhat-developer-hub -i "${CHART_URL}" --version "$CV"
+# collect values - check for existing secret in the target namespace
+if kubectl get secret redhat-developer-hub-postgresql -n "$namespace" &> /dev/null; then
+ PASSWORD=$(kubectl get secret redhat-developer-hub-postgresql -n "$namespace" -o jsonpath="{.data.password}" | base64 -d)
+ echo "Found existing PostgreSQL secret in namespace $namespace, preserving password"
+else
</code_context>
<issue_to_address>
No error handling if secret exists but password field is missing or malformed.
Add checks to ensure the 'password' field exists and is valid base64 before decoding, and handle errors if not.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
rm3l
reviewed
Jun 26, 2025
|
openshift-merge-bot
Bot
merged commit Jun 26, 2025
be64c8a
into
redhat-developer:main
8 checks passed
5 tasks
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
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.



Description of the change
Previously, .rhdh/scripts/install.sh called helm upgrade twice:
This caused two deployment rollouts:
This PR removes the first redundant helm upgrade, keeping only the final call with full values.
Verified:
Which issue(s) does this PR fix or relate to
How to test changes / Special notes to the reviewer
Checklist
Chart.yamlaccording to Semantic Versioning.values.yamland added to the corresponding README.md. The pre-commit utility can be used to generate the necessary content. Usepre-commit run -ato apply changes. The pre-commit Workflow will do this automatically for you if needed.pre-commithook.ct lintcommand.Summary by Sourcery
Simplify the CI install script by removing the redundant Helm upgrade and consolidating deployment into a single invocation with proper password handling.
New Features:
Enhancements: