Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR removes Dagger-based build/migration/release tooling and its config, replaces CI/release flows with Nix/Makefile/GitHub Actions, adds local migration testdata script and benthos-collector version ldflag support, and updates related lint/IDE/config exclusions and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Nix as Nix dev shell
participant Builder as Build steps (make/helm/go)
participant Artifacts as GitHub Artifacts / OCI registry
participant Release as GitHub Release / npm registry
GH->>Nix: run nix develop / set up environment
Nix->>Builder: execute build/package (helm, cross-build, archive)
Builder->>Artifacts: upload artifacts (tgz, tar.gz)
GH->>Artifacts: download artifacts (release job)
GH->>Release: create release, attach assets, publish npm (via reusable workflow)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
AGENTS.md (1)
7-14: Tiny duplication heads-up 🙃Lines 7 and 14 both now say essentially the same thing about
Makefilebeing primary andjustfilebeing seldom used. Not a blocker at all, but if you feel like tidying, you could drop one mention and leave the other as the canonical note.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 7 - 14, Duplicate note about using the Makefile appears in AGENTS.md; remove one of the repeated sentences so the guidance is only stated once. Edit the AGENTS.md content block that mentions "Development commands are run via `Makefile`..." or the earlier line "Use the `Makefile` for all common tasks..." (whichever you prefer to keep) and delete the other redundant sentence, keeping a single canonical mention of `Makefile` and the optional note about `justfile`; ensure the remaining paragraph reads smoothly and no other contextual info is lost..github/workflows/release.yaml (2)
179-188: Consider marking pre-release tags as GitHub pre-releases.
gh release createdefaults to a non-prerelease entry, so av1.2.3-beta.1tag will show up as a regular release on the Releases page and in "latest release" API responses. A tiny conditional keeps the Releases UI honest:🏷️ Suggested tweak
run: | + prerelease_flag="" + case "${GITHUB_REF_NAME}" in + *-beta.*|*-dev.*) prerelease_flag="--prerelease" ;; + esac gh release create "${GITHUB_REF_NAME}" \ --title "${GITHUB_REF_NAME}" \ --generate-notes \ - --verify-tag \ + --verify-tag \ + $prerelease_flag \ dist/benthos-collector_*.tar.gz \ dist/checksums.txt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yaml around lines 179 - 188, The GitHub release step uses gh release create (invoked as gh release create "${GITHUB_REF_NAME}" ...) which always makes a full release; detect prerelease tags (e.g. GITHUB_REF_NAME containing a hyphen like v1.2.3-beta.1) and pass the --prerelease flag when true. Update the workflow to compute a boolean/variable (e.g. PRERELEASE) from GITHUB_REF_NAME and conditionally include --prerelease in the gh release create invocation so prerelease tags are marked correctly in Releases.
67-71: Minor: prefer${{ secrets.GITHUB_TOKEN }}over${{ github.token }}for consistency, and consider using the helm OCI--registry-configflow.Totally cosmetic — both
github.tokenandsecrets.GITHUB_TOKENresolve to the same value, but the rest of the file usessecrets.GITHUB_TOKEN(e.g. the Nix setup steps), so using one style everywhere makes it easier to grep for auth flows. Also, piping the token throughechointohelm registry loginworks fine; if you ever wanted to avoid token material on a pipeline,helm registry login --password-stdinfrom an env var (printenv GH_TOKEN | ...) is a common alternative. Not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yaml around lines 67 - 71, Replace the use of `${{ github.token }}` with `${{ secrets.GITHUB_TOKEN }}` in the "Login to GitHub Container Registry" step and switch the login to use an env-var + stdin or the helm OCI `--registry-config` flow; specifically, update the `helm registry login ghcr.io --username "${{ github.actor }}" --password-stdin` invocation in that step to consume `GITHUB_TOKEN` from the environment (e.g., `echo "${{ secrets.GITHUB_TOKEN }}" | nix develop ... -c ... --password-stdin`) or refactor to use `helm registry login --registry-config <path>` per your OCI config convention so the token usage matches the rest of the workflow.Makefile (1)
55-64: Nice guard, but consider resetting the tree if the check fails.
migrate-check-schemarunsgo generate -x ./openmeter/ent/...which can mutate the tree. On CI that's fine — the job fails and the runner is tossed. Locally though, a dev runningmake migrate-checkwill get a dirty worktree even when they intended only to validate. Totally optional, but a hint in the failure message telling them togit checkout -- openmeter/ent(or the schema-not-in-sync message already suggests regen + commit, which also works) keeps things friendly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 55 - 64, The failure of the migrate-check-schema target leaves generated changes in the working tree; update the migrate-check-schema Makefile recipe (the target named migrate-check-schema that runs go generate -x ./openmeter/ent/...) so that when the check detects differences it also resets the mutated files before exiting (e.g., run a git checkout -- openmeter/ent or git restore --staged/--worktree for openmeter/ent) and update the final echo to advise the user to either regenerate & commit or run the provided reset command to revert local changes.tools/migrate/generate-sqlc-testdata.sh (1)
17-56: Small robustness suggestion: validateVERSIONand clean up on failure.Two nits that'll make this script friendlier for folks hacking on it locally:
- Scratch DB leaks on failure.
set -euo pipefailmeans any failure between creating and dropping the scratch DB (e.g., a broken migration) will leavesqlc_gen_<VERSION>behind, and a re-run will thenDROP DATABASE IF EXISTSbefore recreating — which is mostly fine, but atrapcleanup is nicer and also covers interrupts.VERSIONgoes straight into SQL and a migrate CLI arg. It's expected to be a timestamp, but an early regex check would fail fast with a clear message instead of producing weird SQL errors.🛡️ Suggested tweak
if [[ -z "${VERSION:-}" ]]; then echo "ERROR: VERSION is required (e.g. VERSION=20240826120919)" >&2 exit 1 fi + +if [[ ! "${VERSION}" =~ ^[0-9]+$ ]]; then + echo "ERROR: VERSION must be a numeric migration timestamp (got: ${VERSION})" >&2 + exit 1 +fi @@ SCRATCH_DB="sqlc_gen_${VERSION}" + +cleanup() { + psql -h "${PG_HOST}" -p "${PG_PORT}" -U "${PG_USER}" -d postgres \ + -c "DROP DATABASE IF EXISTS ${SCRATCH_DB};" >/dev/null 2>&1 || true +} +trap cleanup EXITAnd then the explicit drop at the end can go away since the trap handles it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/migrate/generate-sqlc-testdata.sh` around lines 17 - 56, The script can leave the scratch DB (SCRATCH_DB) behind on failures and accepts any VERSION string; add a sanity check and a cleanup trap: validate VERSION with a strict regex (e.g., only digits of expected length) before using it, and register a trap that drops SCRATCH_DB (using the same psql DROP DATABASE IF EXISTS "${SCRATCH_DB}" command) on EXIT/INT/TERM so the DB is removed if migrate or other steps fail; update places that reference VERSION, SCRATCH_DB, psql and migrate to rely on the validated VERSION and remove the final explicit DROP if the trap covers cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yaml:
- Around line 220-227: The JS_SDK_RELEASE_TAG is hard-coded to "latest" which
will publish prereleases under the latest dist-tag; change the Publish NPM
package step to derive JS_SDK_RELEASE_TAG from github.ref_name instead of
hard-coding it (use a conditional expression or a small shell step) so that if
github.ref_name contains "beta" it becomes "beta", if it contains "dev" or
similar it becomes "dev"/"next", otherwise it remains "latest"; update the
environment variable JS_SDK_RELEASE_TAG used by the publish-javascript-sdk
target accordingly.
In `@Makefile`:
- Around line 110-124: Add a new file cmd/benthos-collector/version.go that
declares the package main, a package-level variable var version string, and an
init() that sets version = "unknown" when empty so the Makefile's -X
main.version linker flag can inject the build version; follow the same pattern
used by cmd/server/version.go (declare var version string and initialize to
"unknown" in init()).
---
Nitpick comments:
In @.github/workflows/release.yaml:
- Around line 179-188: The GitHub release step uses gh release create (invoked
as gh release create "${GITHUB_REF_NAME}" ...) which always makes a full
release; detect prerelease tags (e.g. GITHUB_REF_NAME containing a hyphen like
v1.2.3-beta.1) and pass the --prerelease flag when true. Update the workflow to
compute a boolean/variable (e.g. PRERELEASE) from GITHUB_REF_NAME and
conditionally include --prerelease in the gh release create invocation so
prerelease tags are marked correctly in Releases.
- Around line 67-71: Replace the use of `${{ github.token }}` with `${{
secrets.GITHUB_TOKEN }}` in the "Login to GitHub Container Registry" step and
switch the login to use an env-var + stdin or the helm OCI `--registry-config`
flow; specifically, update the `helm registry login ghcr.io --username "${{
github.actor }}" --password-stdin` invocation in that step to consume
`GITHUB_TOKEN` from the environment (e.g., `echo "${{ secrets.GITHUB_TOKEN }}" |
nix develop ... -c ... --password-stdin`) or refactor to use `helm registry
login --registry-config <path>` per your OCI config convention so the token
usage matches the rest of the workflow.
In `@AGENTS.md`:
- Around line 7-14: Duplicate note about using the Makefile appears in
AGENTS.md; remove one of the repeated sentences so the guidance is only stated
once. Edit the AGENTS.md content block that mentions "Development commands are
run via `Makefile`..." or the earlier line "Use the `Makefile` for all common
tasks..." (whichever you prefer to keep) and delete the other redundant
sentence, keeping a single canonical mention of `Makefile` and the optional note
about `justfile`; ensure the remaining paragraph reads smoothly and no other
contextual info is lost.
In `@Makefile`:
- Around line 55-64: The failure of the migrate-check-schema target leaves
generated changes in the working tree; update the migrate-check-schema Makefile
recipe (the target named migrate-check-schema that runs go generate -x
./openmeter/ent/...) so that when the check detects differences it also resets
the mutated files before exiting (e.g., run a git checkout -- openmeter/ent or
git restore --staged/--worktree for openmeter/ent) and update the final echo to
advise the user to either regenerate & commit or run the provided reset command
to revert local changes.
In `@tools/migrate/generate-sqlc-testdata.sh`:
- Around line 17-56: The script can leave the scratch DB (SCRATCH_DB) behind on
failures and accepts any VERSION string; add a sanity check and a cleanup trap:
validate VERSION with a strict regex (e.g., only digits of expected length)
before using it, and register a trap that drops SCRATCH_DB (using the same psql
DROP DATABASE IF EXISTS "${SCRATCH_DB}" command) on EXIT/INT/TERM so the DB is
removed if migrate or other steps fail; update places that reference VERSION,
SCRATCH_DB, psql and migrate to rely on the validated VERSION and remove the
final explicit DROP if the trap covers cleanup.
🪄 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: a2c92f65-d3de-4fad-81cb-2314e9a82ee7
⛔ Files ignored due to path filters (4)
.dagger/go.modis excluded by!**/*.mod.dagger/go.sumis excluded by!**/*.sum,!**/*.sumapi/client/javascript/Makefileis excluded by!api/client/**flake.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (23)
.dagger/.gitattributes.dagger/.gitignore.dagger/build.go.dagger/generate.go.dagger/main.go.dagger/migrate.go.dagger/release.go.dagger/utils.go.dagger/versions.go.dagger/versions_pinned.go.fossa.yaml.github/workflows/ci.yaml.github/workflows/release.yaml.golangci-fast.yaml.golangci.yaml.vscode/default.code-workspaceAGENTS.mdMakefiledagger.jsonflake.nixjustfileopenmeter/testutils/pg_driver.gotools/migrate/generate-sqlc-testdata.sh
💤 Files with no reviewable changes (16)
- .golangci.yaml
- .golangci-fast.yaml
- .dagger/.gitignore
- .vscode/default.code-workspace
- .dagger/.gitattributes
- .dagger/versions_pinned.go
- .dagger/main.go
- .dagger/versions.go
- .dagger/generate.go
- .dagger/utils.go
- dagger.json
- .fossa.yaml
- flake.nix
- .dagger/release.go
- .dagger/migrate.go
- .dagger/build.go
tothandras
force-pushed
the
chore/remove-dagger
branch
from
5b0b075 to
b177189
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/release.yaml (1)
236-248:⚠️ Potential issue | 🟠 MajorKeep prerelease tags off npm
latest, and make beta versions rerunnable.Because lines 5-8 include
-dev/-betatags, the tag branch here still sends prereleases tolatest. Also, non-tag publishes use only the short SHA, so a manual rerun for the same commit tries to republish the same immutable npm version.Suggested metadata tweak
- name: Determine version and npm dist-tag id: meta env: REF_TYPE: ${{ github.ref_type }} + RUN_NUMBER: ${{ github.run_number }} run: | if [[ "$REF_TYPE" == "tag" ]]; then - echo "version=${GITHUB_REF_NAME}" >> "$GITHUB_OUTPUT" - echo "dist-tag=latest" >> "$GITHUB_OUTPUT" + echo "version=${GITHUB_REF_NAME#v}" >> "$GITHUB_OUTPUT" + case "$GITHUB_REF_NAME" in + *-beta.*) echo "dist-tag=beta" >> "$GITHUB_OUTPUT" ;; + *-dev.*) echo "dist-tag=dev" >> "$GITHUB_OUTPUT" ;; + *) echo "dist-tag=latest" >> "$GITHUB_OUTPUT" ;; + esac else short_sha="${GITHUB_SHA:0:12}" - echo "version=1.0.0-beta-${short_sha}" >> "$GITHUB_OUTPUT" + echo "version=1.0.0-beta-${short_sha}.${RUN_NUMBER}" >> "$GITHUB_OUTPUT" echo "dist-tag=beta" >> "$GITHUB_OUTPUT" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yaml around lines 236 - 248, Update the "Determine version and npm dist-tag" step (id: meta) so prerelease tags don't get dist-tag=latest and non-tag beta versions are unique per workflow run: when REF_TYPE=="tag" check GITHUB_REF_NAME for a prerelease marker (e.g., contains '-') and only set dist-tag=latest for normal semver tags, otherwise set dist-tag=beta; when not a tag, append a run-unique value (use GITHUB_RUN_ID or GITHUB_RUN_NUMBER) to the generated beta version (instead of only the short SHA) so reruns produce a new npm version; keep references to GITHUB_REF_NAME and GITHUB_SHA usage in the same step.
🧹 Nitpick comments (2)
tools/migrate/generate-sqlc-testdata.sh (1)
48-93: Consider a cleanup trap so scratch DBs don't linger on failure.If
migrate goto,pg_dump, orsqlc generatefail, the script exits viaset -eand leavessqlc_gen_<VERSION>behind. It's self-healing (next run drops-if-exists), but anEXITtrap keeps things tidy and also protects against Ctrl-C mid-run.♻️ Suggested tweak
echo ">>> Creating scratch database ${SCRATCH_DB}" psql -h "${PG_HOST}" -p "${PG_PORT}" -U "${PG_USER}" -d postgres \ -c "DROP DATABASE IF EXISTS ${SCRATCH_DB};" \ -c "CREATE DATABASE ${SCRATCH_DB};" + +cleanup() { + psql -h "${PG_HOST}" -p "${PG_PORT}" -U "${PG_USER}" -d postgres \ + -c "DROP DATABASE IF EXISTS ${SCRATCH_DB};" >/dev/null 2>&1 || true +} +trap cleanup EXIT…and then you can drop the explicit drop block at lines 91–93.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/migrate/generate-sqlc-testdata.sh` around lines 48 - 93, Add a shell EXIT trap to always drop the scratch DB (using SCRATCH_DB with psql) so failures or Ctrl-C won't leave sqlc_gen_<VERSION> behind; implement the trap near the top of the script (after SCRATCH_DB is set) and call psql -c "DROP DATABASE IF EXISTS ${SCRATCH_DB};" in the trap, then remove the explicit final DROP block after sqlc generate; ensure the trap runs regardless of where migrate, pg_dump, or sqlc generate fail and that any necessary environment variables (PG_HOST, PG_PORT, PG_USER) are available to the trap.AGENTS.md (1)
7-14: Trim the duplicate Makefile/justfile note.Lines 7 and 14 now say the same thing. I’d keep the short Quick Reference entry and remove or shorten the second mention so this guidance doesn’t drift later.
Based on learnings, AGENTS.md should fold guidance into the most relevant section and remove stale or duplicate notes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 7 - 14, The AGENTS.md contains duplicated notes about using the Makefile and justfile (the same guidance appears in the Quick Reference and again later); edit AGENTS.md to keep the short Quick Reference entry that states "Use the `Makefile` for all common tasks. A `justfile` also exists but is seldom used." and remove or condense the later paragraph that repeats this guidance so only one clear reference to `Makefile`/`justfile` remains; ensure the remaining text is folded into the most relevant section and eliminate the second mention to prevent drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yaml:
- Around line 250-263: The sdk-javascript-release job can be manually triggered
from any branch; add a guard so that when the event is workflow_dispatch it only
runs for main branch or tag refs. Update the sdk-javascript-release job (the
block with name "JavaScript SDK Release" and uses:
./.github/workflows/npm-release.yaml) to include an if condition that permits
execution when github.event_name != 'workflow_dispatch' OR (github.ref ==
'refs/heads/main' OR startsWith(github.ref, 'refs/tags/')), thereby preventing
beta releases from arbitrary branches.
In `@tools/migrate/generate-sqlc-testdata.sh`:
- Around line 53-56: The DB_URL construction embeds PG_PASSWORD into the command
line used by migrate, exposing secrets; update the script to avoid including
PG_PASSWORD in DB_URL and rely on the already-exported PGPASSWORD environment
variable (or build a URL without the password) before calling migrate -database
"${DB_URL}" so the password is not visible in process args; adjust references to
DB_URL and the migrate invocation (the lines that set DB_URL and call migrate
-path "${MIGRATIONS_DIR}" -database "${DB_URL}" goto "${VERSION}") accordingly
and ensure any downstream consumers still receive a valid connection string
without the embedded password.
---
Duplicate comments:
In @.github/workflows/release.yaml:
- Around line 236-248: Update the "Determine version and npm dist-tag" step (id:
meta) so prerelease tags don't get dist-tag=latest and non-tag beta versions are
unique per workflow run: when REF_TYPE=="tag" check GITHUB_REF_NAME for a
prerelease marker (e.g., contains '-') and only set dist-tag=latest for normal
semver tags, otherwise set dist-tag=beta; when not a tag, append a run-unique
value (use GITHUB_RUN_ID or GITHUB_RUN_NUMBER) to the generated beta version
(instead of only the short SHA) so reruns produce a new npm version; keep
references to GITHUB_REF_NAME and GITHUB_SHA usage in the same step.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 7-14: The AGENTS.md contains duplicated notes about using the
Makefile and justfile (the same guidance appears in the Quick Reference and
again later); edit AGENTS.md to keep the short Quick Reference entry that states
"Use the `Makefile` for all common tasks. A `justfile` also exists but is seldom
used." and remove or condense the later paragraph that repeats this guidance so
only one clear reference to `Makefile`/`justfile` remains; ensure the remaining
text is folded into the most relevant section and eliminate the second mention
to prevent drift.
In `@tools/migrate/generate-sqlc-testdata.sh`:
- Around line 48-93: Add a shell EXIT trap to always drop the scratch DB (using
SCRATCH_DB with psql) so failures or Ctrl-C won't leave sqlc_gen_<VERSION>
behind; implement the trap near the top of the script (after SCRATCH_DB is set)
and call psql -c "DROP DATABASE IF EXISTS ${SCRATCH_DB};" in the trap, then
remove the explicit final DROP block after sqlc generate; ensure the trap runs
regardless of where migrate, pg_dump, or sqlc generate fail and that any
necessary environment variables (PG_HOST, PG_PORT, PG_USER) are available to the
trap.
🪄 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: 7ec5df1c-a3d5-436d-9474-e7743b7531e8
⛔ Files ignored due to path filters (4)
.dagger/go.modis excluded by!**/*.mod.dagger/go.sumis excluded by!**/*.sum,!**/*.sumapi/client/javascript/Makefileis excluded by!api/client/**flake.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (25)
.dagger/.gitattributes.dagger/.gitignore.dagger/build.go.dagger/generate.go.dagger/main.go.dagger/migrate.go.dagger/release.go.dagger/utils.go.dagger/versions.go.dagger/versions_pinned.go.fossa.yaml.github/workflows/ci.yaml.github/workflows/npm-release.yaml.github/workflows/release.yaml.github/workflows/sdk-javascript-dev-release.yaml.golangci-fast.yaml.golangci.yaml.vscode/default.code-workspaceAGENTS.mdMakefiledagger.jsonflake.nixjustfileopenmeter/testutils/pg_driver.gotools/migrate/generate-sqlc-testdata.sh
💤 Files with no reviewable changes (17)
- .fossa.yaml
- .golangci-fast.yaml
- .vscode/default.code-workspace
- .dagger/.gitignore
- .golangci.yaml
- dagger.json
- .dagger/main.go
- .dagger/versions.go
- .dagger/generate.go
- .github/workflows/sdk-javascript-dev-release.yaml
- .dagger/versions_pinned.go
- .dagger/.gitattributes
- flake.nix
- .dagger/utils.go
- .dagger/migrate.go
- .dagger/build.go
- .dagger/release.go
✅ Files skipped from review due to trivial changes (2)
- openmeter/testutils/pg_driver.go
- .github/workflows/ci.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- justfile
- Makefile
Comment on lines
+250
to
+263
| sdk-javascript-release: | ||
| name: JavaScript SDK Release | ||
| # Runs on both tag pushes (stable/pre-release) and main pushes (per-commit beta). | ||
| # npm's trusted publisher entry is keyed on caller workflow file + environment, | ||
| # so this single caller must serve both channels. | ||
| needs: [sdk-javascript-meta] | ||
| uses: ./.github/workflows/npm-release.yaml | ||
| with: | ||
| version: ${{ needs.sdk-javascript-meta.outputs.version }} | ||
| dist-tag: ${{ needs.sdk-javascript-meta.outputs.dist-tag }} | ||
| permissions: | ||
| contents: read | ||
| id-token: write | ||
| secrets: inherit |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find .github/workflows -name "release.yaml" -o -name "release.yml"Repository: openmeterio/openmeter
Length of output: 95
🏁 Script executed:
cat -n .github/workflows/release.yamlRepository: openmeterio/openmeter
Length of output: 13443
Add a guard to prevent beta JS SDK releases from arbitrary branches.
The workflow_dispatch trigger lets anyone manually kick off this workflow from any branch, which would publish a beta release to npm for that branch. Since beta packages should only come from main or release tags, add the guard to keep things tidy.
Suggested fix
sdk-javascript-release:
name: JavaScript SDK Release
# Runs on both tag pushes (stable/pre-release) and main pushes (per-commit beta).
# npm's trusted publisher entry is keyed on caller workflow file + environment,
# so this single caller must serve both channels.
+ if: github.ref_type == 'tag' || github.ref_name == 'main'
needs: [sdk-javascript-meta]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yaml around lines 250 - 263, The
sdk-javascript-release job can be manually triggered from any branch; add a
guard so that when the event is workflow_dispatch it only runs for main branch
or tag refs. Update the sdk-javascript-release job (the block with name
"JavaScript SDK Release" and uses: ./.github/workflows/npm-release.yaml) to
include an if condition that permits execution when github.event_name !=
'workflow_dispatch' OR (github.ref == 'refs/heads/main' OR
startsWith(github.ref, 'refs/tags/')), thereby preventing beta releases from
arbitrary branches.
Comment on lines
+53
to
+56
| DB_URL="postgres://${PG_USER}:${PG_PASSWORD}@${PG_HOST}:${PG_PORT}/${SCRATCH_DB}?sslmode=disable&x-migrations-table=schema_om" | ||
|
|
||
| echo ">>> Migrating ${SCRATCH_DB} to version ${VERSION}" | ||
| migrate -path "${MIGRATIONS_DIR}" -database "${DB_URL}" goto "${VERSION}" |
Contributor
There was a problem hiding this comment.
Heads-up: password leaks into process args via DB_URL.
migrate -database "${DB_URL}" puts PG_PASSWORD on the command line, where it's visible to anyone running ps on the host (and may land in shell/CI logs if migrate echoes its args on error). For a local dev script against a throwaway scratch DB this is pretty low-risk, but since PGPASSWORD is already exported you could also just rely on that and omit the password from the URL — or at least be aware before anyone reuses this pattern in CI against a non-default credential.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/migrate/generate-sqlc-testdata.sh` around lines 53 - 56, The DB_URL
construction embeds PG_PASSWORD into the command line used by migrate, exposing
secrets; update the script to avoid including PG_PASSWORD in DB_URL and rely on
the already-exported PGPASSWORD environment variable (or build a URL without the
password) before calling migrate -database "${DB_URL}" so the password is not
visible in process args; adjust references to DB_URL and the migrate invocation
(the lines that set DB_URL and call migrate -path "${MIGRATIONS_DIR}" -database
"${DB_URL}" goto "${VERSION}") accordingly and ensure any downstream consumers
still receive a valid connection string without the embedded password.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit