fix(greptimedb-cluster): e2e failed and release cluster chart to ACR failed #185
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces changes to three scripts and a GitHub Actions workflow. A new input parameter, Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
scripts/release/release-charts-to-acr.sh (3)
Line range hint
13-18: Verify the repository URL replacement in Chart.yaml.The sed command hardcodes the OCI registry URL, which differs from the variable
$OCI_REGISTRY_URLdefined at the top of the script. This could cause issues if the registry URL needs to change.Apply this diff to use the variable:
- sed -i 's|https://grafana.github.io/helm-charts|oci://greptime-registry.cn-hangzhou.cr.aliyuncs.com/charts|g' "$dir/Chart.yaml" + sed -i "s|https://grafana.github.io/helm-charts|oci://${OCI_REGISTRY_URL}/charts|g" "$dir/Chart.yaml"
Line range hint
28-31: Add error handling for helm push command.The script should verify the success of the helm push command and handle potential failures.
Apply this diff to add error handling:
echo "Package $chart_name to $packaged_file and push to oci://$OCI_REGISTRY_URL/$OCI_NAMESPACE/$chart_name ..." # Push the packaged chart to the OCI repository, handling the output path. - helm push "${packaged_file}" "oci://$OCI_REGISTRY_URL/$OCI_NAMESPACE" + if ! helm push "${packaged_file}" "oci://$OCI_REGISTRY_URL/$OCI_NAMESPACE"; then + echo "Error: Failed to push chart ${packaged_file} to registry" + exit 1 + fi
Line range hint
1-33: Consider adding script initialization checks and cleanup procedures.To improve script robustness, consider:
- Adding checks for required tools (helm, sed) at script start
- Implementing cleanup of temporary packaged files after successful push
- Adding a trap handler for cleanup on script interruption
Here's a template for the initialization section:
# Add at the beginning of the script set -euo pipefail # Check required tools command -v helm >/dev/null 2>&1 || { echo "helm is required but not installed"; exit 1; } command -v sed >/dev/null 2>&1 || { echo "sed is required but not installed"; exit 1; } # Cleanup function cleanup() { echo "Cleaning up temporary files..." find "$CHARTS_DIR" -type f -name "*.tgz" -delete } # Set trap trap cleanup EXIT.github/workflows/release.yaml (2)
8-12: Consider enhancing the input parameter description.The description could be more specific to help users understand what "ACR" stands for.
release-charts-to-acr: - description: Release the Helm charts to ACR + description: Release the Helm charts to Azure Container Registry (ACR) required: false default: 'false'
Line range hint
71-73: Improve GitHub Pages deployment wait mechanism.The current approach of using
sleep 300is not reliable and wastes CI minutes. Consider using GitHub's deployment API to check the actual deployment status.Replace the sleep with a more robust solution:
- # TODO(zyy17): Maybe it's not a elegant way to wait for GitHub Pages to update. For many scenarios, waiting for 5 minutes is enough. - - name: Wait for deployment of GitHub Pages - run: | - sleep 300 + - name: Wait for GitHub Pages deployment + uses: actions/github-script@v6 + with: + script: | + const owner = context.repo.owner; + const repo = context.repo.repo; + let deployed = false; + + while (!deployed) { + const deployments = await github.rest.repos.listDeployments({ + owner, + repo, + environment: 'github-pages' + }); + + if (deployments.data.length > 0) { + const status = await github.rest.repos.listDeploymentStatuses({ + owner, + repo, + deployment_id: deployments.data[0].id + }); + + if (status.data.length > 0 && status.data[0].state === 'success') { + deployed = true; + } + } + + if (!deployed) { + await new Promise(resolve => setTimeout(resolve, 10000)); + } + }scripts/e2e/greptimedb-cluster.sh (1)
Line range hint
1-1: Consider improving script robustness with proper error handling.The script could benefit from these improvements:
- Add trap for cleanup on script exit
- Clean up port-forwarding output file
Add these lines near the start of the script:
#!/usr/bin/env bash set -e +set -o pipefail + +# Ensure cleanup runs on script exit +trap cleanup EXIT + +# Clean up any existing port-forward output file +rm -f /tmp/connections.outAnd update the cleanup function:
function cleanup() { echo "Cleaning up..." pkill -f "kubectl port-forward" + rm -f /tmp/connections.out }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/release.yaml (2 hunks)
- scripts/e2e/greptimedb-cluster.sh (2 hunks)
- scripts/release/release-charts-to-acr.sh (2 hunks)
🔇 Additional comments (2)
scripts/release/release-charts-to-acr.sh (1)
7-7: Verify the impact of the directory iteration pattern change.The change from
"$CHARTS_DIR"/*/to"$CHARTS_DIR"/*means the script will now process all items in the charts directory, not just subdirectories. This could potentially process files that shouldn't be processed.✅ Verification successful
Directory iteration pattern change is safe
The verification shows that the charts directory contains only three subdirectories (greptimedb-cluster, greptimedb-operator, greptimedb-standalone), each containing a Chart.yaml file. There are no files at the root level that could be incorrectly processed by the new iteration pattern. The change from
"$CHARTS_DIR"/*/to"$CHARTS_DIR"/*is safe in this context.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any unexpected files in the charts directory that might be processed # Expected: Only directories and valid Helm charts echo "Listing all items in charts directory:" ls -la charts/ echo -e "\nVerifying all items are valid Helm charts:" for item in charts/*; do if [ -f "$item" ]; then echo "WARNING: Found file at root level: $item" elif [ -d "$item" ]; then if [ ! -f "$item/Chart.yaml" ]; then echo "WARNING: Directory without Chart.yaml: $item" fi fi doneLength of output: 995
scripts/e2e/greptimedb-cluster.sh (1)
55-58: LGTM: Explicit dependency handling is good.The addition of explicit dependency handling through Grafana repo addition and dependency building is a good practice that helps prevent deployment failures.
zyy17
force-pushed
the
fix/release-failed
branch
from
c6d1e30 to
e69d345
Compare
Summary by CodeRabbit