-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Backport PR #2737 to release/v1.7 for Fix update deps workflow: buf is not found #2739
Backport PR #2737 to release/v1.7 for Fix update deps workflow: buf is not found #2739
Conversation
Signed-off-by: vankichi <kyukawa315@gmail.com>
Deploying vald with Cloudflare Pages
|
📝 WalkthroughWalkthroughThe pull request modifies the workflow file 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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
.github/workflows/update-deps.yaml (2)
Line range hint
44-54
: Consider command execution order and dependenciesThe placement of
make deps
betweenmake update
andmake helm/schema/all
seems logical in terms of dependency management flow. However, we should ensure that:
make deps
doesn't override any changes made bymake update
- Subsequent commands like
helm/schema/all
have their dependencies satisfiedConsider documenting the dependencies between these make targets in the Makefile to make the workflow more maintainable.
Line range hint
71-77
: Fix version comparison in label selection logicThere's a syntax error in the version comparison:
if ${LATEST} eq "1.7"; then
This is incorrect shell syntax and will fail.
Apply this fix:
- if ${LATEST} eq "1.7"; then + if [ "${LATEST}" = "1.7" ]; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/update-deps.yaml
(1 hunks)
🔇 Additional comments (1)
.github/workflows/update-deps.yaml (1)
45-45
: Verify if make deps
resolves the buf tool issue
While adding make deps
is a logical step for dependency management, we should verify if this command actually installs or configures the buf tool as intended.
Let's check the Makefile for the deps
target implementation:
✅ Verification successful
make deps
correctly handles buf tool installation
The make deps
command properly installs and configures the buf tool as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the deps target in Makefile handles buf tool installation
# Search for buf-related commands in the deps target
rg -A 5 'deps:' Makefile
# Check if buf is mentioned in any make targets
rg 'buf' Makefile
Length of output: 256
Description
SSIA
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit