Refactor github actions #2773
Refactor github actions #2773
Conversation
Signed-off-by: kpango <[email protected]>
📝 WalkthroughWalkthroughThis pull request primarily updates the descriptions of various GitHub Actions in YAML files to correct grammatical errors, specifically changing "A action" to "An action." Additionally, some actions have had minor changes to input default values. The overall functionality, inputs, outputs, and control flows of the actions remain unchanged, with no alterations to the logic or error handling. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
|
[CHATOPS:HELP] ChatOps commands.
|
github-actions
bot
added
type/ci
area/makefile
size/S
type/refactoring
priority/low
labels
|
[FORMAT] Failed to format. |
![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
🧹 Outside diff range and nitpick comments (8)
.github/actions/setup-go/action.yaml (1)
34-46: Excellent improvement to the Go setup workflow.The addition of pre-installation checks makes the workflow more efficient by avoiding unnecessary Go installations. This optimization aligns well with GitHub Actions best practices by:
- Reducing execution time when Go is already installed
- Preventing unnecessary downloads and installations
- Providing better visibility into the setup process
.github/actions/e2e-deploy-vald-helm-operator/action.yaml (2)
Line range hint
28-31: Remove default value for required input parameterThe
valdreleaseinput is marked asrequired: truebut also has adefaultvalue. This is contradictory as required parameters should not have default values. The default value should be removed since it's required.valdrelease: description: "Path to the valdrelease.yaml that apply to cluster" required: true - default: "true"
Line range hint
89-108: Enhance shell script robustnessThe deployment script could benefit from additional error handling and validation:
- name: Deploy vald shell: bash id: deploy_vald run: | + set -euo pipefail + + if [ ! -f "${VALDRELEASE}" ]; then + echo "Error: ValdRelease file ${VALDRELEASE} does not exist" + exit 1 + fi + kubectl apply -f ${VALDRELEASE} - sleep 6 + # Wait for CRD to be registered + kubectl wait --for=condition=established crd/valdreleases.vald.vdaas.org --timeout=30s kubectl wait --for=condition=Ready pod -l ${WAIT_FOR_SELECTOR} --timeout=${WAIT_FOR_TIMEOUT} kubectl wait --for=condition=ContainersReady pod -l ${WAIT_FOR_SELECTOR} --timeout=${WAIT_FOR_TIMEOUT} kubectl get pods - podname=`kubectl get pods --selector=${WAIT_FOR_SELECTOR} | tail -1 | awk '{print $1}'` + podname=$(kubectl get pods --selector=${WAIT_FOR_SELECTOR} -o jsonpath='{.items[0].metadata.name}') + if [ -z "${podname}" ]; then + echo "Error: No pods found matching selector ${WAIT_FOR_SELECTOR}" + exit 1 + fi echo "POD_NAME=${podname}" >> $GITHUB_OUTPUT.github/actions/e2e-deploy-vald-readreplica/action.yaml (3)
Line range hint
28-31: Fix incorrect default value for values parameterThe
valuesinput has a default value of "false" which is incorrect for a path parameter. Since it's optional, it should either have an empty string default or a path to a default values file.values: description: "Path to the values.yaml that passed to Helm command." required: false - default: "false" + default: ""
Line range hint
57-108: Reduce code duplication and improve error handlingThe deployment steps for local and remote charts contain duplicated code and could benefit from improved error handling.
- Extract common deployment logic into a composite action or reusable script:
# New file: .github/actions/internal/deploy-vald-readreplica.sh #!/bin/bash set -euo pipefail deploy_vald_readreplica() { local cmd="$1" if [ -n "${VALUES}" ] && [ ! -f "${VALUES}" ]; then echo "Error: Values file ${VALUES} does not exist" exit 1 } eval "${cmd}" echo "Waiting for pods to be ready..." kubectl wait --for=condition=ready pod -l "${WAIT_FOR_SELECTOR}" --timeout="${WAIT_FOR_TIMEOUT}" kubectl get pods podname=$(kubectl get pods --selector="${WAIT_FOR_SELECTOR}" -o jsonpath='{.items[0].metadata.name}') if [ -z "${podname}" ]; then echo "Error: No pods found matching selector ${WAIT_FOR_SELECTOR}" exit 1 fi echo "POD_NAME=${podname}" >> $GITHUB_OUTPUT }
- Update the action to use the common script:
- name: Deploy vald read replica from remote charts shell: bash id: deploy_vald_readreplica_remote if: ${{ inputs.use_local_charts == 'false' }} run: | - helm install \ - --values ${VALUES} \ - ${HELM_EXTRA_OPTIONS} \ - --generate-name charts/vald-readreplica - sleep 3 - kubectl wait --for=condition=ready pod -l ${WAIT_FOR_SELECTOR} --timeout=${WAIT_FOR_TIMEOUT} - kubectl get pods - podname=`kubectl get pods --selector=${WAIT_FOR_SELECTOR} | tail -1 | awk '{print $1}'` - echo "POD_NAME=${podname}" >> $GITHUB_OUTPUT + source .github/actions/internal/deploy-vald-readreplica.sh + cmd="helm install --values ${VALUES} ${HELM_EXTRA_OPTIONS} --generate-name charts/vald-readreplica" + deploy_vald_readreplica "${cmd}"
Line range hint
109-127: Simplify pod name resolution logicThe final step to get the real pod name can be simplified and made more robust.
- name: Get real pod name shell: bash id: get_real_pod_name env: PODNAME_LOCAL_DEPLOY: ${{ steps.deploy_vald_readreplica_local.outputs.POD_NAME }} PODNAME_REMOTE_DEPLOY: ${{ steps.deploy_vald_readreplica_remote.outputs.POD_NAME }} - # Set GITHUB_OUTPUT to the not empty one, PODNAME_LOCAL_DEPLOY or PODNAME_REMOTE_DEPLOY run: | + set -euo pipefail + + pod_name="${PODNAME_LOCAL_DEPLOY:-${PODNAME_REMOTE_DEPLOY}}" + if [ -z "${pod_name}" ]; then + echo "Error: Failed to get pod name from either local or remote deployment" + exit 1 + fi - if [[ -n "${PODNAME_LOCAL_DEPLOY}" ]]; then - echo "POD_NAME=${PODNAME_LOCAL_DEPLOY}" >> $GITHUB_OUTPUT - else - echo "POD_NAME=${PODNAME_REMOTE_DEPLOY}" >> $GITHUB_OUTPUT - fi + echo "POD_NAME=${pod_name}" >> $GITHUB_OUTPUT.github/workflows/chatops.yaml (2)
Line range hint
41-42: Consider including permissions file in the repositoryDownloading the permissions file from the main branch for each job introduces:
- Network dependencies that could cause failures
- Potential security risks if the main branch is compromised
- Inconsistency if permissions change during workflow execution
Consider either:
- Including the permissions file in the repository and using a local copy
- Using GitHub's CODEOWNERS feature for permission management
- Implementing GitHub's environment protection rules
Also applies to: 91-92, 171-172, 284-285, 416-417
152-156: Optimize workflow using reusable workflows and composite actionsThe current implementation has several opportunities for optimization:
- Permission checking logic is duplicated across jobs
- Similar setup steps are repeated
- Some jobs could potentially run in parallel
Consider:
- Creating a reusable workflow for permission checking
- Using composite actions for common setup steps
- Reviewing job dependencies to identify parallelization opportunities
Example composite action for permission checking:
# .github/actions/check-permissions/action.yaml name: 'Check ChatOps Permissions' description: 'Checks if user has required permissions for ChatOps operations' inputs: username: required: true required-policy: required: true outputs: executable: description: 'Whether the user has permission' runs: using: 'composite' steps: - run: | roles=`yq r chatops_permissions.yaml "users.${USERNAME}.role.*"` if [ "$roles" = "" ]; then roles=`yq r chatops_permissions.yaml "default-roles.*"` fi for role in `echo $roles` do if yq r chatops_permissions.yaml "roles.${role}.policies.*" | grep "^${REQUIRED_POLICY}$" > /dev/null 2>&1 ; then echo "EXECUTABLE=true" >> $GITHUB_OUTPUT break fi done env: USERNAME: ${{ inputs.username }} REQUIRED_POLICY: ${{ inputs.required-policy }} shell: bashAlso applies to: 264-269, 396-401
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (24)
.github/actions/deploy-chaos-mesh/action.yaml(1 hunks).github/actions/detect-docker-image-tags/action.yaml(1 hunks).github/actions/determine-docker-image-tag/action.yaml(1 hunks).github/actions/docker-build/action.yaml(1 hunks).github/actions/dump-context/action.yaml(1 hunks).github/actions/e2e-deploy-vald-helm-operator/action.yaml(1 hunks).github/actions/e2e-deploy-vald-readreplica/action.yaml(1 hunks).github/actions/e2e-deploy-vald/action.yaml(1 hunks).github/actions/notify-slack/action.yaml(1 hunks).github/actions/scan-docker-image/action.yaml(1 hunks).github/actions/setup-e2e/action.yaml(1 hunks).github/actions/setup-go/action.yaml(2 hunks).github/actions/setup-helm/action.yaml(2 hunks).github/actions/setup-k3d/action.yaml(2 hunks).github/actions/setup-yq/action.yaml(1 hunks).github/actions/wait-for-docker-image/action.yaml(1 hunks).github/workflows/build-binaries.yaml(0 hunks).github/workflows/chatops.yaml(5 hunks).github/workflows/e2e-code-bench-agent.yaml(0 hunks).github/workflows/format.yaml(0 hunks).github/workflows/helm-lint.yaml(0 hunks).github/workflows/helm.yaml(0 hunks)Makefile.d/helm.mk(0 hunks)Makefile.d/tools.mk(1 hunks)
💤 Files with no reviewable changes (6)
- .github/workflows/helm.yaml
- .github/workflows/build-binaries.yaml
- Makefile.d/helm.mk
- .github/workflows/e2e-code-bench-agent.yaml
- .github/workflows/helm-lint.yaml
- .github/workflows/format.yaml
✅ Files skipped from review due to trivial changes (11)
- .github/actions/notify-slack/action.yaml
- .github/actions/setup-e2e/action.yaml
- .github/actions/scan-docker-image/action.yaml
- .github/actions/deploy-chaos-mesh/action.yaml
- .github/actions/detect-docker-image-tags/action.yaml
- .github/actions/setup-yq/action.yaml
- .github/actions/docker-build/action.yaml
- .github/actions/dump-context/action.yaml
- .github/actions/determine-docker-image-tag/action.yaml
- .github/actions/wait-for-docker-image/action.yaml
- .github/actions/e2e-deploy-vald/action.yaml
🔇 Additional comments (10)
.github/actions/setup-go/action.yaml (4)
17-17: LGTM! Grammar fix approved.
The description now correctly uses "An" instead of "A" before the word "action".
49-49: LGTM! Clear and precise step naming.
The rename from "Check" to "Verify" better reflects the step's purpose of confirming the installed Go version.
44-46: 🛠️ Refactor suggestion
Consider semantic version comparison for Go versions.
The current version comparison (!=) might not handle all version formats correctly. Consider implementing semantic version comparison for more reliable results.
Here's a suggested improvement:
- if: steps.check_go.outputs.go_installed == 'false' || steps.check_go.outputs.installed_version != steps.go_version.outputs.version
+ if: steps.check_go.outputs.go_installed == 'false' || steps.check_go.outputs.installed_version != format('go{0}', steps.go_version.outputs.version)Likely invalid or redundant comment.
34-43: Verify Go version format extraction.
The version extraction logic looks good, but consider making it more robust by explicitly matching the expected Go version format.
Let's verify the version extraction works correctly for different Go versions:
Consider using a more specific pattern match:
- echo "installed_version=$(go version | awk '{print $3}')" >> $GITHUB_OUTPUT
+ echo "installed_version=$(go version | awk '/go version go[0-9]+\.[0-9]+(\.[0-9]+)?/ {print $3}')" >> $GITHUB_OUTPUT✅ Verification successful
Let me check how this version information is used in the workflow to determine if the current extraction is sufficient.
Let me check the entire action file to understand how this version information is used in the workflow.
Current version extraction is sufficient for the workflow's needs
The version extraction is actually working correctly for its intended use case. Looking at the complete action file:
- The extracted version is only used for comparison with
steps.go_version.outputs.version - The comparison is used to determine if Go needs to be installed via
actions/setup-go@v5 - Even if the version extraction captures extra information (like
-rc.1), it won't affect the workflow's functionality since an exact mismatch will trigger a fresh Go installation with the correct version - The final step verifies the Go version explicitly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Test version extraction for different Go version formats
# Test cases
echo "Testing version extraction with different formats:"
echo 'go version go1.20.1 linux/amd64' | awk '{print $3}'
echo 'go version go1.21.0-rc.1 linux/amd64' | awk '{print $3}'
echo 'go version devel +1234567 linux/amd64' | awk '{print $3}'
Length of output: 330
Script:
#!/bin/bash
# Find where the version output is used
rg "installed_version" -A 3 -B 3
Length of output: 32
Script:
#!/bin/bash
# Check the complete action file
cat .github/actions/setup-go/action.yaml
Length of output: 1888
.github/actions/setup-helm/action.yaml (3)
17-17: LGTM: Grammar fix in description
The description now correctly uses "An action" instead of "A action".
34-43: Well-implemented installation check
Good addition of the installation check step. The implementation correctly:
- Uses command -v for portable shell command checking
- Properly captures the installed version
- Sets output variables for conditional execution
44-46: Verify conditional installation logic
The conditional installation and version verification steps look good. Let's verify the version comparison logic works as expected.
Also applies to: 49-52
✅ Verification successful
Let me gather more context about the version checking implementation.
Version comparison logic is correctly implemented
The implementation is well-structured and handles all test cases appropriately:
- Checks if helm is installed using
command -v helm - Extracts installed version using
helm version --short --client - Performs version comparison using exact string matching
- Verifies the installed version after setup
The conditional logic will trigger helm installation when:
- Helm is not installed (
helm_installed=false) - Installed version doesn't match the required version (string comparison)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the helm version comparison logic
# Test cases:
# 1. When helm is not installed
# 2. When helm version matches
# 3. When helm version differs
# Check current implementation
rg -A 2 'if: steps.check_helm.outputs' .github/actions/setup-helm/action.yaml
# Check for similar patterns in other actions
rg -A 2 'helm_installed.*version' .github/actions/
Length of output: 627
Script:
#!/bin/bash
# Check the implementation of version check step
cat .github/actions/setup-helm/action.yaml
# Look for any test files
fd -e yaml -e yml test setup-helm
Length of output: 2124
.github/actions/setup-k3d/action.yaml (2)
70-79: LGTM: Well-structured installation check
The k3d installation check follows the same robust pattern as the Helm action:
- Proper command existence check
- Version extraction and storage
- Clear output variable naming
80-84: Verify make target for k3d installation
The installation step uses make k3d/install. Let's verify this target exists and is properly defined.
✅ Verification successful
Make target k3d/install is properly defined and functional
The k3d/install make target exists in Makefile.d/k3d.mk and is correctly defined. It:
- Creates the binary directory if it doesn't exist
- Downloads and runs the official k3d installation script
- Sets proper executable permissions on the installed binary
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify k3d installation make target exists
# Expected: Find the k3d/install target definition
# Search for k3d/install target in makefiles
rg -A 5 '^k3d/install:' Makefile.d/
Length of output: 324
Makefile.d/tools.mk (1)
264-273: LGTM: Well-implemented yq installation target
The new yq installation target follows the established pattern in the file and includes:
- Proper architecture detection and normalization
- Correct executable permissions setting
- Use of temporary directory
- Consistent target naming and documentation
Deploying vald with
|
| Latest commit: |
54eae6b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0e1c33cc.vald.pages.dev |
| Branch Preview URL: | https://refactor-github-actions-remo.vald.pages.dev |
Signed-off-by: kpango <[email protected]> Co-authored-by: Kiichiro YUKAWA <[email protected]>
Signed-off-by: kpango <[email protected]> Co-authored-by: Yusuke Kato <[email protected]> Co-authored-by: Kiichiro YUKAWA <[email protected]>
Description
SSIA
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
New Features
Bug Fixes
Chores