chore(scripts): add pre-push and pre-merge make targets for local CI parity#30
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Makefile help text and three targets: ChangesPre-Push Validation Commands & Test Readiness
Comment |
Contributor
📊 Unit Test Coverage Report:
|
| Metric | Value |
|---|---|
| Overall Coverage | 45.5% |
| Threshold | 85% |
Coverage by Package
| Package | Coverage |
|---|---|
github.com/LerianStudio/flowker/api |
100.0% |
github.com/LerianStudio/flowker/internal/adapters/http/in/audit |
0.0% |
github.com/LerianStudio/flowker/internal/adapters/http/in/catalog |
60.9% |
github.com/LerianStudio/flowker/internal/adapters/http/in/dashboard |
91.5% |
github.com/LerianStudio/flowker/internal/adapters/http/in/execution |
81.0% |
github.com/LerianStudio/flowker/internal/adapters/http/in/executor_configuration |
70.8% |
github.com/LerianStudio/flowker/internal/adapters/http/in/health |
100.0% |
github.com/LerianStudio/flowker/internal/adapters/http/in/middleware/testutil |
0.0% |
github.com/LerianStudio/flowker/internal/adapters/http/in/middleware |
44.4% |
github.com/LerianStudio/flowker/internal/adapters/http/in/provider_configuration |
33.1% |
github.com/LerianStudio/flowker/internal/adapters/http/in/readyz |
81.3% |
github.com/LerianStudio/flowker/internal/adapters/http/in/webhook |
87.5% |
github.com/LerianStudio/flowker/internal/adapters/http/in/workflow |
69.7% |
github.com/LerianStudio/flowker/internal/adapters/http/in |
33.3% |
github.com/LerianStudio/flowker/internal/adapters/mongodb/dashboard |
43.0% |
github.com/LerianStudio/flowker/internal/adapters/mongodb/execution |
64.8% |
github.com/LerianStudio/flowker/internal/adapters/mongodb/executor_configuration |
36.6% |
github.com/LerianStudio/flowker/internal/adapters/mongodb/provider_configuration |
63.0% |
github.com/LerianStudio/flowker/internal/adapters/mongodb/workflow |
69.2% |
github.com/LerianStudio/flowker/internal/adapters/postgresql/audit |
42.2% |
github.com/LerianStudio/flowker/internal/bootstrap |
40.5% |
github.com/LerianStudio/flowker/internal/services/command |
37.2% |
github.com/LerianStudio/flowker/internal/services/query |
11.2% |
github.com/LerianStudio/flowker/internal/services |
3.2% |
github.com/LerianStudio/flowker/internal/testutil |
63.6% |
github.com/LerianStudio/flowker/pkg/circuitbreaker |
97.6% |
github.com/LerianStudio/flowker/pkg/clock |
0.0% |
github.com/LerianStudio/flowker/pkg/condition |
87.9% |
github.com/LerianStudio/flowker/pkg/contextutil |
0.0% |
github.com/LerianStudio/flowker/pkg/executor/base |
20.0% |
github.com/LerianStudio/flowker/pkg/executor |
39.5% |
github.com/LerianStudio/flowker/pkg/executors/http/auth |
58.2% |
github.com/LerianStudio/flowker/pkg/executors/http |
49.5% |
github.com/LerianStudio/flowker/pkg/executors/midaz |
83.2% |
github.com/LerianStudio/flowker/pkg/executors/s3 |
0.0% |
github.com/LerianStudio/flowker/pkg/executors/tracer |
90.6% |
github.com/LerianStudio/flowker/pkg/executors |
57.1% |
github.com/LerianStudio/flowker/pkg/model |
58.7% |
github.com/LerianStudio/flowker/pkg/net/http |
91.3% |
github.com/LerianStudio/flowker/pkg/pagination |
0.0% |
github.com/LerianStudio/flowker/pkg/safehttp |
90.3% |
github.com/LerianStudio/flowker/pkg/templates/tracer_midaz |
81.5% |
github.com/LerianStudio/flowker/pkg/templates |
0.0% |
github.com/LerianStudio/flowker/pkg/transformation |
67.2% |
github.com/LerianStudio/flowker/pkg/triggers/webhook |
0.0% |
github.com/LerianStudio/flowker/pkg/triggers |
0.0% |
github.com/LerianStudio/flowker/pkg/webhook |
100.0% |
github.com/LerianStudio/flowker/pkg |
90.3% |
Generated by Go PR Analysis workflow
Contributor
🔒 Security Scan Results —
|
| Policy | Status |
|---|---|
| Default non-root user | ✅ Passed |
| No fixable critical/high CVEs | ✅ Passed |
| No high-profile vulnerabilities | ✅ Passed |
| No AGPL v3 licenses | ✅ Passed |
Pre-release Version Check
✅ No unstable version pins found.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 308-310: The pre-merge target currently duplicates the full
pre-push dependency chain; change the pre-merge rule so it depends on pre-push
(instead of listing lint sec build test-unit test-integration test-e2e) to
ensure pre-merge simply invokes the pre-push chain and avoids future
drift—update the Makefile's "pre-merge" target dependency to reference
"pre-push" and keep the existing echo/$(call title1,...) line unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 306-317: The _pre-validations target currently lists lint, sec,
build, test-unit, test-integration as prerequisites so GNU Make can run them in
parallel; change _pre-validations to run those steps sequentially by replacing
the prerequisite list with a recipe that invokes each step in order (e.g., call
$(MAKE) lint && $(MAKE) sec && $(MAKE) build && $(MAKE) test-unit && $(MAKE)
test-integration) so failure short-circuits and the documented fail-fast
ordering is enforced for pre-push and pre-merge which depend on
_pre-validations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/main_test.go`:
- Around line 182-185: The test's container startup uses wait.ForAll assigned to
WaitingFor with WithStartupTimeoutDefault(60 * time.Second), which applies
per-wait defaults and can let the combined waits exceed 60s; replace or change
this to use WithDeadline(60 * time.Second) on the wait.ForAll chain so the
overall startup has a hard 60s cap (locate WaitingFor =
wait.ForAll(...).WithStartupTimeoutDefault(...) and swap to .WithDeadline(60 *
time.Second), keeping the inner waits like wait.ForLog and wait.ForListeningPort
unchanged).
In `@tests/integration/main_test.go`:
- Around line 166-169: The wait strategy uses WithStartupTimeoutDefault(60 *
time.Second) which only sets per-child defaults; change the call on the
WaitingFor block that wraps wait.ForAll(...) to use WithDeadline(60 *
time.Second) instead so the entire ForAll wait has a hard 60s limit; update the
invocation replacing WithStartupTimeoutDefault with WithDeadline while leaving
the child strategies (wait.ForLog and wait.ForListeningPort) intact.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a659f334-6780-46eb-bac4-96a41af6740d
📒 Files selected for processing (2)
tests/e2e/main_test.gotests/integration/main_test.go
7 tasks
alexgarzao
added a commit
that referenced
this pull request
May 21, 2026
The fail-fast ordering documented above _pre-validations (lint → sec → build → test-unit → test-integration) only holds when prerequisites run serially. Without .NOTPARALLEL, `make -j pre-push` or any caller that inherits parallel MAKEFLAGS would spin up testcontainers in parallel with lint, defeating the "no time wasted on MongoDB if lint fails" guarantee. Resolves CodeRabbit finding on _pre-validations parallel execution: #30 (comment)
alexgarzao
added a commit
that referenced
this pull request
May 21, 2026
The previous WithStartupTimeoutDefault(60s) call sets the per-strategy
fallback timeout — the duration each child wait inherits when it lacks
its own explicit timeout. With wait.ForAll combining log + port checks,
each child could independently consume up to 60s, allowing the total
ForAll wait to exceed 60s on failure.
Switch to WithDeadline(60s), which caps the entire ForAll operation at
60s regardless of individual child behavior. This matches the original
intent of the comment block ("60s overall startup cap") and the
testcontainers-go v0.42.0 MultiStrategy semantics.
Identical fix applied to both tests/e2e/main_test.go and
tests/integration/main_test.go (the wait config is duplicated across
the two suites).
Resolves CodeRabbit findings:
- #30 (comment)
- #30 (comment)
…parity Two tiers matching the natural `git push` vs `pr merge` rhythm: - pre-push: lint, security, build, unit, integration. Skips e2e (slow, CI runs it anyway). Catches lint/sec/unit/integration failures locally and saves a CI round-trip. ~2-3 min, requires Docker. - pre-merge: pre-push + e2e for full CI parity. ~7-10 min. Dependency order is fail-fast: cheap checks first, testcontainers last. Updates help text and groups under a new "Pre-Push Validation Commands" section.
…idations target Addresses CodeRabbit's drift concern (pre-merge was duplicating pre-push's full dep chain) without the UX cost of `pre-merge: pre-push test-e2e` — which would print pre-push's "safe to push, run make pre-merge for full CI parity" message in the middle of a pre-merge invocation. The shared validation chain now lives on a private _pre-validations target. Both pre-push and pre-merge depend on it, so they cannot drift. Neither's recipe runs during the other's execution.
The Postgres testcontainer in both tests/integration and tests/e2e used
`wait.ForListeningPort("5432/tcp")`, which only checks the TCP port. Postgres
opens the port early during init but is not yet ready to accept connections,
so the first `bootstrap.AuditDatabaseManager.Connect` ping races and gets
`connection reset by peer` from the still-initializing server. This has been
flaking integration and e2e CI on develop for at least a week (visible on
runs since 2026-05-08).
Canonical fix: Postgres logs "database system is ready to accept connections"
twice during startup — once for the transient init server, once for the real
external server. Waiting for occurrence #2 (combined with the port check)
guarantees the DB will not reject incoming connections.
Scope note: this fix lives on the Makefile pre-push branch because the
Integration Tests job is part of the same CI surface and was blocking this
PR's merge despite being a pre-existing develop bug. Other open PRs (#23,
#28) will also benefit once they rebase onto a develop that includes this.
The fail-fast ordering documented above _pre-validations (lint → sec → build → test-unit → test-integration) only holds when prerequisites run serially. Without .NOTPARALLEL, `make -j pre-push` or any caller that inherits parallel MAKEFLAGS would spin up testcontainers in parallel with lint, defeating the "no time wasted on MongoDB if lint fails" guarantee. Resolves CodeRabbit finding on _pre-validations parallel execution: #30 (comment)
The previous WithStartupTimeoutDefault(60s) call sets the per-strategy
fallback timeout — the duration each child wait inherits when it lacks
its own explicit timeout. With wait.ForAll combining log + port checks,
each child could independently consume up to 60s, allowing the total
ForAll wait to exceed 60s on failure.
Switch to WithDeadline(60s), which caps the entire ForAll operation at
60s regardless of individual child behavior. This matches the original
intent of the comment block ("60s overall startup cap") and the
testcontainers-go v0.42.0 MultiStrategy semantics.
Identical fix applied to both tests/e2e/main_test.go and
tests/integration/main_test.go (the wait config is duplicated across
the two suites).
Resolves CodeRabbit findings:
- #30 (comment)
- #30 (comment)
alexgarzao
force-pushed
the
chore/makefile-prepush-targets
branch
from
c09a317 to
980be2f
Compare
Audit of the Makefile recipes uncovered four targets where multi-line
shell blocks used `cmd; echo "[ok]"` instead of `cmd && echo "[ok]"`
or `set -e`. With the `;` separator, the shell's exit code is the
echo's exit code (always 0) — so a failing golangci-lint, gosec, or
node validator would silently report "Linting completed successfully"
and let `make pre-push` / `make pre-merge` pass anyway, defeating the
local-CI-parity contract this PR is supposed to deliver.
Targets fixed:
- `lint` — golangci-lint exit code now propagates.
- `sec` — gosec exit code now propagates (both the
install branch and the run branch).
- `validate-api-docs` — both node validators now fail the recipe.
- `generate-docs-all` — the `$(MAKE) generate-docs 2>&1 | grep`
pipe now preserves the inner make's exit
code via a portable capture-and-replay
pattern (POSIX sh; avoids `set -o pipefail`
which is not portable to dash).
Verified with a stub `golangci-lint` returning 1:
before: `make lint` reported OK and exited 0.
after: `make lint` exits with make's Error 1.
This makes `make pre-push` / `make pre-merge` truly fail-fast — which
is what the original PR comment block above _pre-validations claims.
Triggered on PRs that touch the Makefile (or the workflow itself).
Runs `make -n pre-push` and `make -n pre-merge` to catch parse errors
or removed dependencies, then verifies the `.NOTPARALLEL` directive
still covers _pre-validations / pre-push / pre-merge.
Scope is intentionally narrow:
- Catches: Makefile syntax breakage, removed targets that pre-push
or pre-merge depend on, accidental removal of .NOTPARALLEL.
- Does NOT catch: runtime failures inside recipes (lint, tests,
build). Those remain the job of go-combined-analysis.yml.
Runs ~5s, paths-filtered to Makefile changes only, so it adds no
noise to PRs that don't touch the build surface. Complements the
heavier shared workflow rather than duplicating its work.
…e fix The previous commit (cb142d2) made `make sec` honor gosec's exit code, surfacing 3 pre-existing findings that golangci-lint silently excludes via .golangci.yml (G101 and G115 are blanket-suppressed there). Standalone gosec doesn't read .golangci.yml, so `make sec` was now stricter than CI — breaking the local CI parity contract pre-push / pre-merge promise. Suppressing per-site (with justifications) restores parity AND keeps G101/G115 ACTIVE so any future legitimate hit gets caught (unlike the blanket exclusion in .golangci.yml). Suppressions: internal/adapters/http/in/middleware/apikey.go:17 HeaderAPIKey = "X-API-Key" G101 false positive — HTTP header NAME, not a credential value. (Identical pattern to apiKeyHeader fixed via #nosec in PR #25.) pkg/executors/http/auth/types.go:30 TypeOIDCClientCredentials Type = "oidc_client_credentials" G101 false positive — Type discriminator (auth flow identifier), not a credential value. internal/adapters/postgresql/audit/repository.go:275 builder.Limit(uint64(limit + 1)) G115 with documented justification — filter.Limit is guaranteed 1..100 by the query layer (search_audit_logs.go:52-56 clamps <=0 to default 20 and >100 to 100), so `limit + 1` cannot overflow int and the uint64 cast is safe. Reference added to the comment so future readers can verify the bound. Tradeoff vs blanket -exclude=G101,G115 in the gosec CLI: 3 file edits vs 1 Makefile line. Chose per-site for precision — the suppression lives next to the code it justifies and survives future refactors.
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
2 participants
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
Adds two Makefile targets that match the natural
git pushvspr mergerhythm:make pre-push— lint + security + build + unit + integration tests. Skips e2e (slow, CI runs it anyway). Catches lint/sec/unit/integration failures locally and saves a CI round-trip. ~2-3 min, requires Docker.make pre-merge— pre-push + e2e for full CI parity. ~7-10 min. Run before opening a PR or when reproducing a CI-only failure locally.Why two tiers and not one
If a single
make pre-pushran e2e (~5+ min/run), most people would skip it and we'd lose all local validation. Two tiers gives:pre-push).pre-merge).Why two and not three
A
pre-committier is already covered by the project'smake setup-git-hooksscript. Three tiers is one more than most people can keep straight.Design notes
lint → sec → build → test-unit → test-integration → (test-e2e). Cheapest first; if lint fails, make stops there and no time is wasted spinning up testcontainers.pre-pushandpre-mergedeclare make dependencies on the existinglint,sec,build,test-unit,test-integration,test-e2etargets. No duplication of logic.make helpbetween Code Quality and Docker commands.Test plan
make -n pre-pushresolves to the expected dep chain.make -n pre-mergeresolves to the expected dep chain includingtest-e2e.make helpshows the new section.make pre-pushandmake pre-mergeagainst develop (requires Docker; reviewer can validate locally).