refactor(makefile): migrate to just#4284
Conversation
📝 WalkthroughWalkthroughThis PR converts the project's primary task runner from ChangesMigration to Just Task Runner
Sequence DiagramsequenceDiagram
participant Developer
participant Make as Make (Shim)
participant Just as Just
participant Shell as Shell Script
participant Tool as Build Tool
Developer->>Make: make test
Make->>Just: just test
Just->>Shell: execute go-test.sh
Shell->>Tool: go test ./...
Tool-->>Shell: test results
Shell-->>Just: exit code
Just-->>Make: exit code
Make-->>Developer: output
Developer->>Make: make generate
Make->>Just: just generate
Just->>Shell: patch-oapi-templates.sh
Shell->>Tool: go module resolution & patch
Tool-->>Shell: patched template
Shell->>Just: continue to go generate
Just->>Tool: go generate ./...
Tool-->>Just: generated code
Just-->>Developer: completed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The PR spans a significant architectural shift across the entire developer tooling layer. While individual changes are relatively straightforward delegation patterns, reviewers need to verify: (1) all shell scripts have proper error handling and environment variable defaults; (2) the make-to-just mapping is complete and bidirectional (no orphaned make targets or uncovered just tasks); (3) backward compatibility fallbacks work correctly; (4) documentation accurately reflects the new workflows; and (5) the justfile updates don't introduce regressions in the generate/release flows. The heterogeneity of file types (shell scripts, Makefiles, markdown, justfile) and the interconnected nature of the changes demand careful cross-file reasoning. Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 8: In the AGENTS.md sentence "OpenMeter is a metering and billing
platform with usage based pricing and access control.", change the compound
adjective "usage based" to the hyphenated form "usage-based" so it reads
"OpenMeter is a metering and billing platform with usage-based pricing and
access control."
In `@docs/seeder.md`:
- Line 19: Update the sentence that reads "In some cases, you might want to fine
tune the total number of events and at what rate they are ingested:" to
hyphenate "fine-tune" (i.e., "fine-tune the total number of events...") so the
verb phrase is grammatically correct; locate the exact string in docs/seeder.md
and replace "fine tune" with "fine-tune".
In `@Makefile`:
- Around line 114-118: The usage text for the make target is inconsistent with
the required check for JS_SDK_RELEASE_TAG; either make the tag optional by
providing a default (e.g., set JS_SDK_RELEASE_TAG to a default value with
Makefile variable defaulting like using "?=" or shell parameter expansion) and
remove the exit-on-empty check in the publish-javascript-sdk target, or update
the usage message to show the variable is required (remove square brackets and
state it as required) and keep the existing validation; apply the same fix
consistently for the other variable referenced near the top
(JS_SDK_RELEASE_VERSION) so usage and validation match.
In `@tools/tasks/check-clean-paths.sh`:
- Around line 12-17: The conditional in check-clean-paths.sh uses git diff
--quiet -- "$@" which only checks unstaged changes; change that invocation to
git diff HEAD --quiet -- "$@" so the if-statement (the line containing git diff
--quiet -- "$@") detects both staged and unstaged modifications relative to
HEAD, leaving the rest of the block (git --no-pager diff, git ls-files --others
--exclude-standard, echo "$message", exit 1) unchanged.
In `@tools/tasks/check-postgres.sh`:
- Around line 4-9: The script sets PGPASSWORD from PGPASSWORD only and ignores
POSTGRES_PASSWORD; update the password fallback logic used before invoking psql
so PGPASSWORD falls back to POSTGRES_PASSWORD then to "postgres" (i.e., make the
PGPASSWORD expansion consider POSTGRES_PASSWORD as the next fallback), leaving
the rest of the psql invocation (POSTGRES_HOST, POSTGRES_USER, POSTGRES_DB, and
the psql command) unchanged.
In `@tools/tasks/e2e-local.sh`:
- Around line 22-23: The health-check curl invocation currently uses curl
--retry ... http://localhost:30000/healthz which treats HTTP 4xx/5xx as success;
update the curl command used in the health check to include --fail so that HTTP
error responses cause a non-zero exit (and interact correctly with
--retry/--retry-max-time) — locate the curl invocation in the health check block
and add the --fail flag to that invocation.
In `@tools/tasks/package-helm-chart.sh`:
- Around line 12-19: Validate the CHART input before using it: ensure the
variable chart contains only allowed characters (e.g., alphanumerics, dashes,
and underscores) and does not include path separators or traversal segments,
then construct chart_dir="deploy/charts/${chart}" and check that this directory
exists before calling helm-docs or helm dependency update; if validation fails
or chart_dir is missing, print an error and exit non‑zero to fail fast. Ensure
the checks occur before the helm-docs and helm dependency update invocations
mentioned in the script (chart_dir, helm-docs, helm dependency update).
In `@tools/tasks/quickstart-local.sh`:
- Line 20: Update the health-check curl invocation in quickstart-local.sh (the
line invoking curl against http://localhost:40000/healthz) to include the --fail
flag so that HTTP 4xx/5xx responses are treated as errors and retried; keep the
existing --retry, --retry-max-time and --retry-all-errors options and ensure the
curl command fails on non-2xx responses so startup waits for a healthy /healthz
before proceeding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cab4de92-6a4e-433b-b11d-7a610a3b7111
⛔ Files ignored due to path filters (2)
api/client/javascript/Makefileis excluded by!api/client/**api/client/python/Makefileis excluded by!api/client/**
📒 Files selected for processing (22)
AGENTS.mdMakefileREADME.mdapi/spec/Makefileapi/spec/README.mddeploy/charts/Makefiledocs/seeder.mde2e/Makefilejustfilequickstart/Makefiletools/tasks/api-spec-generate.shtools/tasks/archive-benthos-collector-release.shtools/tasks/build-benthos-collector-release.shtools/tasks/check-clean-paths.shtools/tasks/check-config-fresh.shtools/tasks/check-postgres.shtools/tasks/e2e-local.shtools/tasks/go-test-all.shtools/tasks/go-test.shtools/tasks/package-helm-chart.shtools/tasks/patch-oapi-templates.shtools/tasks/quickstart-local.sh
|
|
||
| Use the `Makefile` for all common tasks. A `justfile` also exists but is seldom used. | ||
| Use `just` for common development tasks. The `Makefile` is a compatibility shim that forwards old targets to `just`. | ||
| OpenMeter is a metering and billing platform with usage based pricing and access control. |
There was a problem hiding this comment.
Minor grammar fix: hyphenate "usage-based"
When a compound adjective comes before a noun (like "pricing"), it should be hyphenated.
✏️ Suggested fix
-OpenMeter is a metering and billing platform with usage based pricing and access control.
+OpenMeter is a metering and billing platform with usage-based pricing and access control.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| OpenMeter is a metering and billing platform with usage based pricing and access control. | |
| OpenMeter is a metering and billing platform with usage-based pricing and access control. |
🧰 Tools
🪛 LanguageTool
[grammar] ~8-~8: Use a hyphen to join words.
Context: ...metering and billing platform with usage based pricing and access control. ## Ti...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` at line 8, In the AGENTS.md sentence "OpenMeter is a metering and
billing platform with usage based pricing and access control.", change the
compound adjective "usage based" to the hyphenated form "usage-based" so it
reads "OpenMeter is a metering and billing platform with usage-based pricing and
access control."
| SEEDER_LOG=true just seed | ||
| ``` | ||
|
|
||
| In some cases, you might want to fine tune the total number of events and at what rate they are ingested: |
There was a problem hiding this comment.
Minor grammar nit: hyphenate "fine-tune".
"Fine-tune" used as a verb phrase should be hyphenated.
✏️ Proposed fix
-In some cases, you might want to fine tune the total number of events and at what rate they are ingested:
+In some cases, you might want to fine-tune the total number of events and at what rate they are ingested:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| In some cases, you might want to fine tune the total number of events and at what rate they are ingested: | |
| In some cases, you might want to fine-tune the total number of events and at what rate they are ingested: |
🧰 Tools
🪛 LanguageTool
[grammar] ~19-~19: Use a hyphen to join words.
Context: ...` In some cases, you might want to fine tune the total number of events and at w...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/seeder.md` at line 19, Update the sentence that reads "In some cases,
you might want to fine tune the total number of events and at what rate they are
ingested:" to hyphenate "fine-tune" (i.e., "fine-tune the total number of
events...") so the verb phrase is grammatically correct; locate the exact string
in docs/seeder.md and replace "fine tune" with "fine-tune".
| if [ -z "$$JS_SDK_RELEASE_TAG" ]; then \ | ||
| echo "ERROR: JS_SDK_RELEASE_TAG is required"; \ | ||
| echo "Usage: JS_SDK_RELEASE_VERSION=1.2.3 make publish-javascript-sdk [JS_SDK_RELEASE_TAG=beta]"; \ | ||
| exit 1; \ | ||
| fi; \ |
There was a problem hiding this comment.
Usage message says optional, but code requires it
The usage hint shows [JS_SDK_RELEASE_TAG=beta] in brackets (suggesting it's optional), but the script then exits with an error if it's not set. Either the check should be removed (making it truly optional with a default), or the usage message should indicate it's required.
✏️ Option A: Make tag truly optional with a default
- if [ -z "$$JS_SDK_RELEASE_TAG" ]; then \
- echo "ERROR: JS_SDK_RELEASE_TAG is required"; \
- echo "Usage: JS_SDK_RELEASE_VERSION=1.2.3 make publish-javascript-sdk [JS_SDK_RELEASE_TAG=beta]"; \
- exit 1; \
- fi; \
+ JS_SDK_RELEASE_TAG="$${JS_SDK_RELEASE_TAG:-latest}"; \✏️ Option B: Fix usage message to show it's required
- echo "Usage: JS_SDK_RELEASE_VERSION=1.2.3 make publish-javascript-sdk [JS_SDK_RELEASE_TAG=beta]"; \
+ echo "Usage: JS_SDK_RELEASE_VERSION=1.2.3 JS_SDK_RELEASE_TAG=beta make publish-javascript-sdk"; \And similarly for line 111-112.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ -z "$$JS_SDK_RELEASE_TAG" ]; then \ | |
| echo "ERROR: JS_SDK_RELEASE_TAG is required"; \ | |
| echo "Usage: JS_SDK_RELEASE_VERSION=1.2.3 make publish-javascript-sdk [JS_SDK_RELEASE_TAG=beta]"; \ | |
| exit 1; \ | |
| fi; \ | |
| JS_SDK_RELEASE_TAG="$${JS_SDK_RELEASE_TAG:-latest}"; \ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 114 - 118, The usage text for the make target is
inconsistent with the required check for JS_SDK_RELEASE_TAG; either make the tag
optional by providing a default (e.g., set JS_SDK_RELEASE_TAG to a default value
with Makefile variable defaulting like using "?=" or shell parameter expansion)
and remove the exit-on-empty check in the publish-javascript-sdk target, or
update the usage message to show the variable is required (remove square
brackets and state it as required) and keep the existing validation; apply the
same fix consistently for the other variable referenced near the top
(JS_SDK_RELEASE_VERSION) so usage and validation match.
| if ! git diff --quiet -- "$@" || [ -n "$(git ls-files --others --exclude-standard -- "$@")" ]; then | ||
| git --no-pager diff -- "$@" | ||
| git ls-files --others --exclude-standard -- "$@" | ||
| echo "$message" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
git diff --quiet misses staged changes — consider git diff HEAD --quiet instead
git diff --quiet -- "$@" only compares the working tree to the index (unstaged changes). If a generated file gets staged (e.g., via git add) before this script runs, the check quietly passes even though the tree is dirty relative to HEAD.
Swapping to git diff HEAD --quiet covers both staged and unstaged changes with no other change needed:
🛠️ Proposed fix
-if ! git diff --quiet -- "$@" || [ -n "$(git ls-files --others --exclude-standard -- "$@")" ]; then
- git --no-pager diff -- "$@"
+if ! git diff HEAD --quiet -- "$@" || [ -n "$(git ls-files --others --exclude-standard -- "$@")" ]; then
+ git --no-pager diff HEAD -- "$@"
git ls-files --others --exclude-standard -- "$@"
echo "$message"
exit 1
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/tasks/check-clean-paths.sh` around lines 12 - 17, The conditional in
check-clean-paths.sh uses git diff --quiet -- "$@" which only checks unstaged
changes; change that invocation to git diff HEAD --quiet -- "$@" so the
if-statement (the line containing git diff --quiet -- "$@") detects both staged
and unstaged modifications relative to HEAD, leaving the rest of the block (git
--no-pager diff, git ls-files --others --exclude-standard, echo "$message", exit
1) unchanged.
| PGPASSWORD="${PGPASSWORD:-postgres}" \ | ||
| psql \ | ||
| -h "${POSTGRES_HOST:-127.0.0.1}" \ | ||
| -U "${POSTGRES_USER:-postgres}" \ | ||
| "${POSTGRES_DB:-postgres}" \ | ||
| -c "SELECT version();" || { |
There was a problem hiding this comment.
Support POSTGRES_PASSWORD as a fallback for password resolution.
Nice script overall, but this currently ignores POSTGRES_PASSWORD, which is the variable defined for Postgres in docker-compose.base.yaml (Line 108 onward). In non-default setups this can fail even when Postgres is up.
Suggested patch
-PGPASSWORD="${PGPASSWORD:-postgres}" \
+PGPASSWORD="${PGPASSWORD:-${POSTGRES_PASSWORD:-postgres}}" \
psql \
-h "${POSTGRES_HOST:-127.0.0.1}" \
-U "${POSTGRES_USER:-postgres}" \
"${POSTGRES_DB:-postgres}" \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| PGPASSWORD="${PGPASSWORD:-postgres}" \ | |
| psql \ | |
| -h "${POSTGRES_HOST:-127.0.0.1}" \ | |
| -U "${POSTGRES_USER:-postgres}" \ | |
| "${POSTGRES_DB:-postgres}" \ | |
| -c "SELECT version();" || { | |
| PGPASSWORD="${PGPASSWORD:-${POSTGRES_PASSWORD:-postgres}}" \ | |
| psql \ | |
| -h "${POSTGRES_HOST:-127.0.0.1}" \ | |
| -U "${POSTGRES_USER:-postgres}" \ | |
| "${POSTGRES_DB:-postgres}" \ | |
| -c "SELECT version();" || { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/tasks/check-postgres.sh` around lines 4 - 9, The script sets PGPASSWORD
from PGPASSWORD only and ignores POSTGRES_PASSWORD; update the password fallback
logic used before invoking psql so PGPASSWORD falls back to POSTGRES_PASSWORD
then to "postgres" (i.e., make the PGPASSWORD expansion consider
POSTGRES_PASSWORD as the next fallback), leaving the rest of the psql invocation
(POSTGRES_HOST, POSTGRES_USER, POSTGRES_DB, and the psql command) unchanged.
| # wait for sink-worker to be ready | ||
| curl --retry 10 --retry-max-time 120 --retry-all-errors http://localhost:30000/healthz |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -type f -name "e2e-local.sh"Repository: openmeterio/openmeter
Length of output: 236
🏁 Script executed:
cat -n tools/tasks/e2e-local.sh | head -30Repository: openmeterio/openmeter
Length of output: 783
🌐 Web query:
curl --fail flag HTTP error codes 4xx 5xx behavior
💡 Result:
The curl --fail (or -f) flag makes curl exit with error code 22 and suppress the response body output for HTTP responses with status codes 400 or greater (all 4xx and 5xx errors). By default, curl treats HTTP responses as successful regardless of status code and outputs the body. For 4xx (client errors like 400, 401, 403, 404) and 5xx (server errors like 500, 502, 503), --fail causes: - Exit code: 22 (CURLE_HTTP_RETURNED_ERROR) - No response body output - Error message to stderr (e.g., "curl: (22) The requested URL returned error: 404") Example: curl --fail http://httpbin.org/status/404 # Outputs: curl: (22) The requested URL returned error: 404 # Exit code: 22 Since curl 7.76.0 (2021), --fail-with-body provides the same failure behavior but outputs/saves the response body. Note: Not 100% reliable for 401/407 due to authentication retries. 1xx, 2xx, 3xx are unaffected.
Citations:
- 1: https://curl.se/docs/manpage.html
- 2: https://curl.se/docs/manpage.html?pubDate=20250316
- 3: https://curl.se/libcurl/c/libcurl-errors.html
Add --fail to catch HTTP errors in the health check.
Without --fail, curl treats HTTP error responses (like 503) as successful transfers and exits with code 0. This means if the health endpoint returns an error status, the script happily moves on to run go test before the stack is actually ready. Adding --fail makes curl exit with error code 22 on any 4xx/5xx response, which plays nicely with the --retry logic to keep probing until things are genuinely healthy.
Suggested fix
-curl --retry 10 --retry-max-time 120 --retry-all-errors http://localhost:30000/healthz
+curl --fail --silent --show-error \
+ --retry 10 --retry-max-time 120 --retry-all-errors \
+ http://localhost:30000/healthz📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # wait for sink-worker to be ready | |
| curl --retry 10 --retry-max-time 120 --retry-all-errors http://localhost:30000/healthz | |
| # wait for sink-worker to be ready | |
| curl --fail --silent --show-error \ | |
| --retry 10 --retry-max-time 120 --retry-all-errors \ | |
| http://localhost:30000/healthz |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/tasks/e2e-local.sh` around lines 22 - 23, The health-check curl
invocation currently uses curl --retry ... http://localhost:30000/healthz which
treats HTTP 4xx/5xx as success; update the curl command used in the health check
to include --fail so that HTTP error responses cause a non-zero exit (and
interact correctly with --retry/--retry-max-time) — locate the curl invocation
in the health check block and add the --fail flag to that invocation.
| chart_dir="deploy/charts/${chart}" | ||
| version_no_v="${version#v}" | ||
|
|
||
| mkdir -p build/helm | ||
| helm-docs --log-level info -s file -c "$chart_dir" \ | ||
| -t "deploy/charts/template.md" \ | ||
| -t "$chart_dir/README.tmpl.md" | ||
| helm dependency update "$chart_dir" |
There was a problem hiding this comment.
Validate CHART before using it as a path.
Right now a typo or a value like ../.. makes helm-docs and helm dependency update operate outside deploy/charts, which is a pretty sharp edge for a release task. Reject traversal patterns and fail fast if the chart directory doesn't exist.
Possible hardening
-chart_dir="deploy/charts/${chart}"
+case "$chart" in
+ */*|*..*)
+ echo "ERROR: CHART must be a chart name under deploy/charts" >&2
+ exit 1
+ ;;
+esac
+
+chart_dir="deploy/charts/${chart}"
+if [ ! -d "$chart_dir" ]; then
+ echo "ERROR: unknown chart: $chart" >&2
+ exit 1
+fi
+
version_no_v="${version#v}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/tasks/package-helm-chart.sh` around lines 12 - 19, Validate the CHART
input before using it: ensure the variable chart contains only allowed
characters (e.g., alphanumerics, dashes, and underscores) and does not include
path separators or traversal segments, then construct
chart_dir="deploy/charts/${chart}" and check that this directory exists before
calling helm-docs or helm dependency update; if validation fails or chart_dir is
missing, print an error and exit non‑zero to fail fast. Ensure the checks occur
before the helm-docs and helm dependency update invocations mentioned in the
script (chart_dir, helm-docs, helm dependency update).
|
|
||
| "${compose[@]}" up -d --force-recreate | ||
|
|
||
| curl --retry 10 --retry-max-time 120 --retry-all-errors http://localhost:40000/healthz |
There was a problem hiding this comment.
Add --fail to the health-check curl to actually retry on HTTP errors.
Without --fail, curl does not consider an HTTP error response code as a failure — for example, if a server replies 404 or 5xx and the reply is fully received, that is not an error to curl. Consequently, --retry-all-errors won't retry on 4xx/5xx responses unless --fail is combined with it. As it stands, if /healthz returns a 5xx during startup, curl exits 0 and go test kicks off against an unhealthy stack.
🔧 Proposed fix
-curl --retry 10 --retry-max-time 120 --retry-all-errors http://localhost:40000/healthz
+curl --retry 10 --retry-max-time 120 --retry-all-errors --fail http://localhost:40000/healthz📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| curl --retry 10 --retry-max-time 120 --retry-all-errors http://localhost:40000/healthz | |
| curl --retry 10 --retry-max-time 120 --retry-all-errors --fail http://localhost:40000/healthz |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/tasks/quickstart-local.sh` at line 20, Update the health-check curl
invocation in quickstart-local.sh (the line invoking curl against
http://localhost:40000/healthz) to include the --fail flag so that HTTP 4xx/5xx
responses are treated as errors and retried; keep the existing --retry,
--retry-max-time and --retry-all-errors options and ensure the curl command
fails on non-2xx responses so startup waits for a healthy /healthz before
proceeding.
1 participant
Overview
Changes Makefile to Justfile + task scripts
Notes for reviewer
Summary by CodeRabbit
Documentation
justcommands instead ofmakeRefactor
maketojustfor improved task management and consistency