Backport PR #2773 to release/v1.7 for Refactor github actions #2779
Conversation
Signed-off-by: kpango <[email protected]> Co-authored-by: Kiichiro YUKAWA <[email protected]>
Deploying vald with
|
| Latest commit: |
ce1af2b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://00f19c08.vald.pages.dev |
| Branch Preview URL: | https://backport-release-v1-7-refact-5usm.vald.pages.dev |
📝 WalkthroughWalkthroughThis pull request primarily focuses on updating the descriptions of various GitHub Actions to correct grammatical errors. The changes span multiple action configuration files, with each update changing "A action" to "An action." Additionally, some actions had minor modifications in their input parameters, particularly regarding default values. The overall functionality and structure of the actions remain unchanged, with no alterations to their logic or execution steps. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
Makefile.d/tools.mk (1)
264-274: LGTM! Consider adding error handling for the curl command.The yq installation target follows the established pattern and correctly handles architecture differences. However, the curl command could benefit from error handling.
Consider adding error checking:
$(BINDIR)/yq: mkdir -p $(BINDIR) $(eval DARCH := $(subst aarch64,arm64,$(ARCH))) cd $(TEMP_DIR) \ - && curl -fsSL https://github.com/mikefarah/yq/releases/download/$(YQ_VERSION)/yq_$(OS)_$(subst x86_64,amd64,$(shell echo $(DARCH) | tr '[:upper:]' '[:lower:]')) -o $(BINDIR)/yq \ + && curl -fsSL https://github.com/mikefarah/yq/releases/download/$(YQ_VERSION)/yq_$(OS)_$(subst x86_64,amd64,$(shell echo $(DARCH) | tr '[:upper:]' '[:lower:]')) -o $(BINDIR)/yq || (echo "Failed to download yq" && exit 1) \ && chmod a+x $(BINDIR)/yq.github/actions/setup-go/action.yaml (1)
34-43: Consider improving version extraction robustnessWhile the Go installation check works, the version extraction could be more robust:
- The current
awk '{print $3}'assumes a specific format ofgo versionoutput- The version comparison in the subsequent step might not handle semantic versioning correctly
Consider this more robust version extraction:
- echo "installed_version=$(go version | awk '{print $3}')" >> $GITHUB_OUTPUT + echo "installed_version=$(go version | sed -n 's/.*go\([0-9.]*\).*/\1/p')" >> $GITHUB_OUTPUT.github/actions/setup-k3d/action.yaml (2)
70-84: Improve robustness of k3d installation checkThe new installation check and conditional installation logic is a good improvement that prevents unnecessary reinstallation. However, consider these enhancements:
- Add explicit error handling for the make command
- Implement proper semantic version comparison
Consider this enhancement:
if command -v k3d &> /dev/null; then echo "k3d_installed=true" >> $GITHUB_OUTPUT - echo "installed_version=$(k3d version --short | sed 's/^v//')" >> $GITHUB_OUTPUT + INSTALLED_VERSION=$(k3d version --short | sed 's/^v//') + echo "installed_version=${INSTALLED_VERSION}" >> $GITHUB_OUTPUT else echo "k3d_installed=false" >> $GITHUB_OUTPUT fi if steps.check_k3d.outputs.k3d_installed == 'false' || steps.check_k3d.outputs.installed_version != steps.k3d_version.outputs.tag shell: bash run: | - make k3d/install + if ! make k3d/install; then + echo "Failed to install k3d" + exit 1 + fi
Security: Confirmed sensitive information exposure across multiple actions
The verification reveals a pattern of exposing sensitive cluster information across multiple GitHub Actions:
.github/actions/setup-k3d/action.yaml: Prints full KUBECONFIG contents.github/actions/setup-e2e/action.yaml: Executes full cluster-info dump.github/actions/deploy-chaos-mesh/action.yaml: Executes full cluster-info dumpRecommendations:
- Replace
cat $KUBECONFIGwith a filtered output showing only non-sensitive fields- Replace
kubectl cluster-info dumpwithkubectl cluster-infowhich provides basic connectivity information- If detailed debugging is needed, implement proper log redaction or limit the scope of information exposed
🔗 Analysis chain
Line range hint
124-125: Security: Consider limiting sensitive information exposureThe action currently prints sensitive information to logs:
- Full KUBECONFIG contents
- Complete cluster-info dump
Consider:
- Masking sensitive values in KUBECONFIG output
- Limiting cluster-info dump to essential information only
Let's check if this information exposure is consistent with other k8s-related actions:
Also applies to: 131-132
Let me check the content of these files to understand the context better.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar patterns in other action files rg -l 'cluster-info dump' .github/actions/ rg -l 'cat.*KUBECONFIG' .github/actions/Length of output: 247
Script:
#!/bin/bash # Check the content of related action files cat .github/actions/setup-e2e/action.yaml cat .github/actions/deploy-chaos-mesh/action.yamlLength of output: 5655
.github/actions/setup-helm/action.yaml (1)
34-43: LGTM! Robust Helm installation check implementation.The implementation correctly:
- Uses POSIX-compliant command checking
- Handles both installed and not installed cases
- Extracts version information when available
Consider adding error handling for the version extraction command in case it fails:
if command -v helm &> /dev/null; then echo "helm_installed=true" >> $GITHUB_OUTPUT - echo "installed_version=$(helm version --short --client | awk '{print $2}' | sed 's/^v//')" >> $GITHUB_OUTPUT + if ! version=$(helm version --short --client | awk '{print $2}' | sed 's/^v//'); then + echo "::warning::Failed to extract Helm version" + echo "installed_version=" >> $GITHUB_OUTPUT + else + echo "installed_version=$version" >> $GITHUB_OUTPUT + fi else
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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
- .github/workflows/e2e-code-bench-agent.yaml
- Makefile.d/helm.mk
- .github/workflows/helm-lint.yaml
- .github/workflows/format.yaml
✅ Files skipped from review due to trivial changes (13)
- .github/actions/setup-e2e/action.yaml
- .github/actions/docker-build/action.yaml
- .github/actions/deploy-chaos-mesh/action.yaml
- .github/actions/dump-context/action.yaml
- .github/actions/wait-for-docker-image/action.yaml
- .github/actions/notify-slack/action.yaml
- .github/actions/e2e-deploy-vald/action.yaml
- .github/actions/e2e-deploy-vald-readreplica/action.yaml
- .github/actions/detect-docker-image-tags/action.yaml
- .github/actions/e2e-deploy-vald-helm-operator/action.yaml
- .github/actions/scan-docker-image/action.yaml
- .github/actions/setup-yq/action.yaml
- .github/actions/determine-docker-image-tag/action.yaml
🔇 Additional comments (8)
.github/workflows/chatops.yaml (2)
27-27: LGTM! Good security practice to prevent potential infinite loops.
The condition if: github.event.comment.user.login != 'vdaas-ci' prevents the bot from responding to its own comments, which could otherwise lead to infinite loops.
Also applies to: 77-77, 152-152, 264-264, 396-396
30-31: LGTM! Good practice using standardized container environment.
Using a pre-configured container image (ghcr.io/vdaas/vald/vald-ci-container:nightly) ensures consistent environment across all jobs and eliminates the need for redundant setup steps.
Let's verify the container image exists and is from a trusted source:
Also applies to: 80-81, 157-158, 270-271, 402-403
.github/actions/setup-go/action.yaml (3)
17-17: LGTM! Grammar fix looks good.
The description now correctly uses "An" instead of "A" before the word "action".
49-49: LGTM! Step name change is appropriate.
The rename from "Check" to "Verify" better reflects the step's purpose of confirming the final Go installation state.
44-46: Verify version comparison logic
The condition steps.check_go.outputs.installed_version != steps.go_version.outputs.version might not work as expected due to format differences:
installed_versionmight include "go" prefix- Version formats might differ (e.g., "1.20" vs "1.20.0")
Let's verify the version comparison logic:
✅ Verification successful
Let me check the actual implementation of the version comparison logic in the action.
Looking at the implementation, I can now generate the final response:
Version comparison is correctly implemented
The version comparison logic is actually correct because:
- The
installed_versionis extracted usinggo version | awk '{print $3}'which gets just the version part (e.g., "go1.20.1" -> "go1.20.1") - The
go_versionstep usesmake version/gowhich would output the same format as it's the source of truth - The final verification step ensures the correct version is installed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how go version is formatted in different contexts
echo "Testing go version output format:"
go version
echo -e "\nChecking make version/go output format:"
make version/go 2>/dev/null || echo "Note: make command not available in sandbox"
Length of output: 392
Script:
#!/bin/bash
# Check the implementation of check_go step
echo "Checking check_go step implementation:"
cat .github/actions/setup-go/action.yaml
# Search for any test files that might show version comparison
echo -e "\nSearching for test files:"
fd -e test.yaml -e test.yml -e test.js -e test.sh . .github/actions/setup-go/
Length of output: 2122
.github/actions/setup-k3d/action.yaml (1)
17-17: LGTM: Grammar fix in description
The grammatical correction from "A action" to "An action" is appropriate.
.github/actions/setup-helm/action.yaml (2)
17-17: LGTM! Grammar correction applied.
The description now correctly uses "An" instead of "A" before the word "action".
49-52: LGTM! Clean version verification implementation.
The command uses appropriate flags to produce clean, parseable output.
|
[CHATOPS:HELP] ChatOps commands.
|
|
[FORMAT] Failed to format. |
vankichi
deleted the
backport/release/v1.7/refactor/github-actions/remove-setup-go
branch
Description
SSIA
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
New Features
Bug Fixes
Chores