Add nightly.changelog.com #46
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a nightly backend and proxy across the stack: Varnish VCL, new 204 health probe, Dagger wiring, benchmarks/just targets, VTC and acceptance tests for nightly, and CI changes to publish/deploy on version tags; some production acceptance tests removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Varnish
participant NightlyVCL as Nightly VCL
participant NightlyBackend as Nightly Backend
Note right of Varnish: default.vcl includes nightly-backend.vcl and new directors/probes
Client->>Varnish: Request (Host=NIGHTLY_HOST or /nightly_health)
Varnish->>NightlyVCL: vcl_recv matches nightly
alt Method not GET/HEAD/PURGE
NightlyVCL-->>Varnish: synth 405 with Allow header
Varnish-->>Client: 405 + Allow
else Allowed method
NightlyVCL->>NightlyBackend: backend fetch (/ or /health or asset)
NightlyBackend-->>Varnish: 200/204 + headers
Varnish->>NightlyVCL: vcl_backend_response sets storage=memory, TTL/grace/keep
NightlyVCL->>Varnish: add CORS and x-backend-nightly on deliver
Varnish-->>Client: Response (possibly cached)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (22)
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}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
justfile (1)
63-75: Production acceptance tests will fail—addnightly_hostvariable.The production test runs
test/acceptance/*.hurl, which includestest/acceptance/nightly.hurl. This file references{{nightly_host}}at multiple locations (lines 3, 19, 30, 47, 58), but the production configuration doesn't define this variable, unlike the local test configuration which sets--variable nightly_host=nightly.changelog.com. Add the missing variable to avoid test failures.
🧹 Nitpick comments (3)
test/acceptance/periodic/region.hurl (1)
27-40: Hardcoded file size may cause test brittleness.The exact byte count assertion
bytes count == 113283951will fail if this MP3 file is ever re-encoded or replaced. If this is a canonical test file that should never change, this is fine. Otherwise, consider using a range assertion or removing the exact size check.test/acceptance/feeds.hurl (1)
142-150: Consider adding a comment explaining why this test is skipped.The
/feed/endpoint test is disabled withskip: true, but there's no explanation for why. Adding a brief comment would help future maintainers understand if this is a known bug, pending feature, or intentional exclusion.varnish/vcl/nightly-backend.vcl (1)
45-61: Verify that the 1-minute TTL is appropriate for nightly content.The backend response configuration:
ttl = 1m(fresh for 1 minute)grace = 1d(serve stale while refreshing for 1 day)keep = 7d(keep for conditional requests for 7 days)- Forces memory storage
The 1-minute TTL means content will be refreshed frequently. Verify this aligns with the expected update frequency of nightly content. If nightly content changes less frequently (e.g., daily), consider increasing the TTL to reduce backend load.
Consider whether the TTL should be longer based on the actual update frequency of nightly content. For daily updates, a TTL of several hours might be more appropriate.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.github/workflows/_github.yml.github/workflows/_namespace.ymlREADME.mdcontainer/justfiledagger/main.gojustfiletest/acceptance/admin.hurltest/acceptance/assets.hurltest/acceptance/feeds.hurltest/acceptance/health.hurltest/acceptance/homepage.hurltest/acceptance/nightly.hurltest/acceptance/periodic/region.hurltest/acceptance/production/assets.hurltest/acceptance/production/feed.hurltest/acceptance/production/feeds.hurltest/vtc/assets-backend.vtctest/vtc/fly/client-ip.vtctest/vtc/nightly-backend.vtcvarnish/vcl/default.vclvarnish/vcl/nightly-backend.vcl
💤 Files with no reviewable changes (3)
- test/acceptance/production/feeds.hurl
- test/acceptance/production/assets.hurl
- test/acceptance/production/feed.hurl
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-30T13:45:33.339Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/feeds-backend.vcl:19-109
Timestamp: 2025-12-30T13:45:33.339Z
Learning: In the varnish/vcl/ directory, prefer explicit code repetition over complex regular expressions for maintainability and readability, even if it increases lines of code. This guideline applies to all .vcl files within varnish/vcl/ (and its subdirectories) to keep patterns straightforward and easier to review.
Applied to files:
varnish/vcl/nightly-backend.vclvarnish/vcl/default.vcl
📚 Learning: 2025-12-30T13:53:53.196Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/fly/client-ip.vcl:3-16
Timestamp: 2025-12-30T13:53:53.196Z
Learning: In the varnish/vcl/fly/client-ip.vcl file, the client IP determination logic only logs the selected IP via std.log and does not store it in a request header. This is intentional because the health-checker reads directly from log output rather than from request headers.
Applied to files:
varnish/vcl/nightly-backend.vclvarnish/vcl/default.vcltest/vtc/fly/client-ip.vtc
📚 Learning: 2025-12-30T13:48:47.598Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/news-mp3.vcl:97-97
Timestamp: 2025-12-30T13:48:47.598Z
Learning: In varnish/vcl/news-mp3.vcl, the ASSETS_HOST environment variable should NOT have a fallback or default value. The code should fail if ASSETS_HOST is not set, following a fail-fast pattern for required configuration.
Applied to files:
varnish/vcl/nightly-backend.vclvarnish/vcl/default.vcltest/vtc/assets-backend.vtc
📚 Learning: 2025-12-30T17:03:18.040Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: just/fly.just:94-98
Timestamp: 2025-12-30T17:03:18.040Z
Learning: In Fly.io deployments, volumes created in the primary region are automatically replicated to instances in other regions by the platform. Therefore, it's correct to create a volume (like `varnish_file_cache`) only in the primary region while setting environment variables (like `VARNISH_FILE_SIZE`) globally for all machines.
Applied to files:
test/vtc/assets-backend.vtc
🔇 Additional comments (26)
test/vtc/fly/client-ip.vtc (1)
1-86: LGTM!The test file is well-structured with comprehensive coverage for client IP determination logic across 6 test cases. The formatting adjustments maintain consistency throughout the file.
container/justfile (1)
48-53: LGTM!The addition of
nightly_hostvariable properly supports the new nightly backend tests, and removing the production test glob from local acceptance testing aligns with the PR's goal of reorganizing tests to run production checks after deployment.README.md (1)
21-21: LGTM!Roadmap correctly updated to reflect the completed nightly backend work with the PR reference.
test/acceptance/feeds.hurl (1)
1-61: LGTM!Excellent coverage of the feed caching lifecycle including cold load, cached load with staleness detection, and purge verification. The duration assertions provide useful performance guardrails.
dagger/main.go (4)
59-62: LGTM!The struct extension cleanly adds the new proxy fields following the existing pattern.
102-109: LGTM!Default proxy configurations are well-structured. The port allocation (5000 app, 5010 assets, 5020 feeds, 5030 nightly) follows a logical sequence.
218-234: LGTM!Proxy initialization follows the established error-handling pattern consistently.
250-273: LGTM!The procfile correctly wires up all four TLS terminators (app, assets, feeds, nightly), and the environment variable setup properly exposes the backend configuration for Varnish VCL consumption.
test/vtc/assets-backend.vtc (1)
1-155: LGTM!Formatting improvements enhance readability. The test logic and assertions remain intact, and the environment variable handling correctly follows the fail-fast pattern for
ASSETS_HOST.test/acceptance/health.hurl (1)
16-38: Inconsistency between AI summary and actual code.The AI summary states "Replaces the assets_health test block with a nightly_health block," but the code shows both
assets_health(lines 16-22) andnightly_health(lines 32-38) test blocks are present. This is not an issue with the code itself, but the summary does not accurately describe the changes.varnish/vcl/default.vcl (3)
23-48: LGTM! Proper fail-fast configuration.The dynamic directors are properly configured without fallback values for environment variables, following the fail-fast pattern. The timeout configurations are appropriate for the respective backends.
41-48: Verify that BACKEND_NIGHTLY_FQDN is set in the deployment environment.The nightly director depends on the
BACKEND_NIGHTLY_FQDNenvironment variable. Following the fail-fast pattern, there's correctly no fallback value. This variable is configured in deployment viadagger/main.goand should be present in all environments where the Varnish config is deployed.
23-30: Ensure BACKEND_ASSETS_FQDN is set in all deployment environments.The assets director correctly depends on
BACKEND_ASSETS_FQDNwith no fallback value, following the fail-fast pattern. The variable is configured in Dagger deployments viadagger/main.go. Verify it's consistently set in all environments where Varnish runs..github/workflows/_github.yml (2)
26-37: LGTM! Tag-based deployment flow is correct.The workflow now properly:
- Publishes and deploys only when a version tag (starting with
v) is pushed- Runs production acceptance tests after deployment to avoid test failures on unreleased code
This aligns with the PR objective to reorganize acceptance tests so production checks run after deploying the new version.
39-45: LGTM! Artifact upload guarantees test reports.The
always()condition ensures test reports are uploaded even if tests fail, which improves debugging and visibility. The wildcard patterntmp/test-acceptance-*appropriately captures all acceptance test artifacts..github/workflows/_namespace.yml (2)
30-41: LGTM! Consistent tag-based deployment.The Namespace.so workflow mirrors the GitHub workflow changes, maintaining consistency across CI environments. The tag-based trigger and production test ordering align with the PR objectives.
43-49: LGTM! Consistent artifact handling.The artifact upload configuration matches the GitHub workflow, ensuring test reports are captured consistently across both CI platforms.
test/acceptance/assets.hurl (3)
64-114: LGTM! Comprehensive MP3 range request testing.The test suite properly validates:
- Full MP3 file delivery with size verification (8,575,592 bytes)
- First chunk (bytes 0-1023)
- Middle chunk (bytes 1024-2047) with Content-Range validation
- Last chunk (bytes -1024) with Content-Range validation
- All chunks are exactly 1024 bytes
- Disk storage for large media files
- CORS headers for all responses
This ensures proper HTTP range request support for media streaming.
1-29: LGTM! PURGE flow validation is thorough.The test correctly validates the complete PURGE lifecycle:
- Initial GET with cache headers
- PURGE with token authentication
- GET after PURGE showing cache miss
The
{{purge_token}}variable is securely configured via 1Password integration for production (op://pipely/purge/credential) and environment variables for local testing.
50-52: POST rejection for static assets is correctly implemented in assets-backend.vcl.The VCL code at lines 14-16 of assets-backend.vcl rejects POST requests (along with any method not in GET|HEAD|OPTIONS|PURGE) with a 405 Method Not Allowed response, which matches the test expectation. The vcl_synth subroutine also correctly sets the Allow header for these responses.
test/vtc/nightly-backend.vtc (1)
1-126: LGTM! Comprehensive VTC test coverage.The VTC test properly validates:
- Health endpoint mapping (
/nightly_health→/health)- Method restrictions (GET/HEAD/PURGE allowed, POST/OPTIONS rejected with 405)
- Allow header in 405 responses
- CORS headers for static assets
- Memory-based storage via logexpect
- Multiple asset types (CSS, PNG)
This provides strong verification of the nightly backend VCL logic.
varnish/vcl/nightly-backend.vcl (4)
3-22: LGTM! Proper request routing and method filtering.The request handling correctly:
- Routes requests for NIGHTLY_HOST or /nightly_health to the nightly backend
- Rejects non-GET/HEAD/PURGE methods with 405
- Maps /nightly_health to /health and bypasses cache
- Sets appropriate tracking headers (x-backend-fqdn, x-backend-nightly)
The explicit method regex follows the learning to prefer explicit patterns over complex regex.
Based on learnings, the code correctly uses explicit patterns and no fallback values for environment variables.
24-31: LGTM! Correct 405 response synthesis.The synth handler properly returns 405 with an Allow header listing the permitted methods (GET, HEAD, PURGE), which conforms to RFC 7231.
33-40: LGTM! CORS header enables cross-origin requests.The wildcard CORS header (
access-control-allow-origin: *) is appropriate for static nightly content that needs to be accessible from various origins.
3-61: Verify required environment variables are configured.The VCL depends on the following environment variables:
NIGHTLY_HOSTBACKEND_NIGHTLY_HOSTBACKEND_NIGHTLY_PORTBACKEND_NIGHTLY_FQDNThese correctly have no fallback values, following the fail-fast pattern for required configuration. Ensure all variables are set in deployment configurations.
test/acceptance/nightly.hurl (1)
1-66: LGTM! Comprehensive nightly backend test coverage.The test suite properly validates:
- Cold and warm cache behavior with appropriate duration assertions (<1s cold, <100ms cached)
- Cache metadata (ttl, grace, keep, storage)
- CORS headers for static assets
- Both GET and HEAD methods
- Storage backend (memory) behavior
The
{{nightly_host}}variable is defined incontainer/justfileasnightly.changelog.comand is properly configured in the test environment.
gerhard
force-pushed
the
add-nightly-backend
branch
from
be78656 to
3faa999
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
varnish/vcl/default.vcl (1)
51-68: Reorder includes for alphabetical consistency.The new probe files (
backend-health-200.vcl,backend-health-204.vcl) and nightly backend routing (nightly-backend.vcl) are correctly included, andnightly-backend.vclcontains proper backend routing logic. However, the includes are not in alphabetical order:fly/region.vcl(position 15) should appear afterfly/request-id.vcl(position 9) to maintain alphabetical sorting.Correction needed
Move
fly/region.vclto come afterfly/request-id.vcl:include "fly/client-ip.vcl"; include "fly/region.vcl"; include "fly/request-id.vcl";
🧹 Nitpick comments (1)
test/acceptance/nightly.hurl (1)
28-44: Consider parameterizing the hardcoded date.The hardcoded date
/2025/12/30/in the URL and body assertion may require updates as time passes. Consider using a template variable or a recent-but-stable date that's less likely to break.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.github/workflows/_github.yml.github/workflows/_namespace.ymlREADME.mdcontainer/justfiledagger/main.gojustfiletest/acceptance/admin.hurltest/acceptance/assets.hurltest/acceptance/feeds.hurltest/acceptance/health.hurltest/acceptance/homepage.hurltest/acceptance/nightly.hurltest/acceptance/periodic/region.hurltest/acceptance/production/assets.hurltest/acceptance/production/feed.hurltest/acceptance/production/feeds.hurltest/vtc/assets-backend.vtctest/vtc/fly/client-ip.vtctest/vtc/nightly-backend.vtcvarnish/vcl/backend-health-200.vclvarnish/vcl/backend-health-204.vclvarnish/vcl/default.vclvarnish/vcl/nightly-backend.vcl
💤 Files with no reviewable changes (3)
- test/acceptance/production/assets.hurl
- test/acceptance/production/feeds.hurl
- test/acceptance/production/feed.hurl
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/_github.yml
- test/vtc/fly/client-ip.vtc
- justfile
- varnish/vcl/nightly-backend.vcl
- test/acceptance/assets.hurl
- test/vtc/nightly-backend.vtc
- README.md
- test/acceptance/periodic/region.hurl
- test/vtc/assets-backend.vtc
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-30T13:45:33.339Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/feeds-backend.vcl:19-109
Timestamp: 2025-12-30T13:45:33.339Z
Learning: In the varnish/vcl/ directory, prefer explicit code repetition over complex regular expressions for maintainability and readability, even if it increases lines of code. This guideline applies to all .vcl files within varnish/vcl/ (and its subdirectories) to keep patterns straightforward and easier to review.
Applied to files:
varnish/vcl/backend-health-200.vclvarnish/vcl/backend-health-204.vclvarnish/vcl/default.vcl
📚 Learning: 2025-12-30T13:48:47.598Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/news-mp3.vcl:97-97
Timestamp: 2025-12-30T13:48:47.598Z
Learning: In varnish/vcl/news-mp3.vcl, the ASSETS_HOST environment variable should NOT have a fallback or default value. The code should fail if ASSETS_HOST is not set, following a fail-fast pattern for required configuration.
Applied to files:
varnish/vcl/default.vcltest/acceptance/health.hurl
📚 Learning: 2025-12-30T13:53:53.196Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/fly/client-ip.vcl:3-16
Timestamp: 2025-12-30T13:53:53.196Z
Learning: In the varnish/vcl/fly/client-ip.vcl file, the client IP determination logic only logs the selected IP via std.log and does not store it in a request header. This is intentional because the health-checker reads directly from log output rather than from request headers.
Applied to files:
varnish/vcl/default.vcl
🔇 Additional comments (21)
varnish/vcl/backend-health-200.vcl (1)
1-32: LGTM!The probe rename to
backend_health_200with explicit.expected_response = 200is clear and consistent with the newbackend_health_204probe pattern. The probe configuration is well-documented and uses appropriate values..github/workflows/_namespace.yml (2)
38-49: LGTM!Good improvements:
- Production acceptance tests now run only after tag deployments, avoiding test failures during deployment transitions.
- The
always()condition on artifact archival ensures test reports are captured even when tests fail, aiding debugging.
30-36: The deployment directory pathfly.io/cdn-2025-02-25exists with the requiredjustfile. The workflow correctly sequences deployment before production acceptance tests.test/acceptance/feeds.hurl (2)
1-8: LGTM!Clean RSS feed test with appropriate header assertions for Cloudflare, Varnish, caching, and content type validation.
9-61: Comprehensive cache lifecycle testing with a note on timing assertions.The cache behavior tests are well-structured, covering cold load, stale serving, background refresh, purge, and post-purge fetch. The assertions for
cache-statuscomponents (region, origin, ttl, grace, keep, storage) provide good coverage.Note that
duration < 1000andduration < 500assertions may occasionally flake in CI environments with network variability. Consider whether these thresholds provide sufficient margin, or if they should be configurable via variables similar todelay_ms.dagger/main.go (4)
59-63: LGTM!The
FeedsProxyandNightlyProxyfields are properly added to thePipelystruct, maintaining consistency with the existingAppProxyandAssetsProxypattern.
102-109: LGTM!The default proxy configurations follow the established
PORT:FQDN:HOSTpattern:
5010:changelog.place:cdn.changelog.comfor assets5020:feeds.changelog.place:for feeds5030:changelog-nightly-2023-10-10.fly.dev:nightly.changelog.comfor nightlyPort assignments (5000, 5010, 5020, 5030) are sequential and non-conflicting.
218-234: LGTM!Proxy initialization follows the established pattern with proper error handling. The
NewProxyhelper validates the input format and returns structured proxy data.
250-273: LGTM!The procfile and environment variable wiring correctly integrates the nightly backend:
- TLS exterminator entries for all four backends (app, assets, feeds, nightly)
- Backend environment variables (
BACKEND_NIGHTLY_FQDN,BACKEND_NIGHTLY_HOST,BACKEND_NIGHTLY_PORT) follow the established naming conventionNIGHTLY_HOSTmirrors the pattern used forASSETS_HOSTcontainer/justfile (3)
48-53: LGTM!The
nightly_hostvariable addition enables proper nightly backend testing, and the test scope adjustment totest/acceptance/*.hurl test/acceptance/local/*.hurlcorrectly separates local tests from production-only tests.
60-61: LGTM!The optional
hostparameter enables Host header override in benchmarks, which is necessary for testing host-based routing through the local Varnish instance (e.g.,bench-assets-4-localandbench-nightly-4-local).
66-109: LGTM!The benchmark targets follow a consistent naming convention (
bench-{type}-{step}-{layer}) and cover the full request path:
- origin - direct backend
- tls-proxy - through TLS terminator
- fly - through Fly.io CDN
- local - through local Varnish
The nightly benchmarks (lines 99-109) mirror the existing app/assets/feed patterns.
test/acceptance/health.hurl (2)
16-22: LGTM!The
assets_healthtest correctly expects HTTP 200, validates Varnish routing, and assertsorigin=assetswith the matching comment.
32-38: LGTM!The
nightly_healthtest correctly expects HTTP 204 (matching thebackend_health_204probe configuration), validates Varnish routing, and assertsorigin=nightlywith the matching comment.varnish/vcl/backend-health-204.vcl (1)
1-32: LGTM!The
backend_health_204probe is correctly configured for backends that return 204 No Content for health checks. The structure mirrorsbackend_health_200with only the.expected_responsediffering, which aligns with the learning to prefer explicit code over complex abstractions in VCL files.varnish/vcl/default.vcl (3)
13-21: LGTM!The app director correctly uses
backend_health_200probe, which aligns with the renamed probe expecting HTTP 200 responses.
23-39: LGTM!The
assetsandfeedsdynamic directors are properly configured withbackend_health_200probe. The timeout values (first_byte_timeout=10s, connect_timeout=10s, between_bytes_timeout=60s) are appropriate for static asset and feed serving.
41-49: LGTM!The
nightlydynamic director correctly usesbackend_health_204probe, which matches the nightly backend's health endpoint returning HTTP 204 No Content. The configuration mirrors the feeds director with appropriate timeout values.test/acceptance/nightly.hurl (3)
1-16: LGTM! Comprehensive cold-cache test.The cold-cache test for the index page is well-structured, validating response time, routing through Fly and Varnish, and all relevant cache-status fields.
17-27: LGTM! Effective cache hit validation.Testing the same endpoint twice to validate cache behavior is excellent practice. The sub-100ms assertion and cache hit verification are appropriate.
56-66: LGTM! Correct syntax and good HEAD method coverage.This test segment correctly places expected response headers after the HTTP status line. Testing both GET and HEAD methods for static assets is good practice.
gerhard
force-pushed
the
add-nightly-backend
branch
from
3faa999 to
b20a271
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
test/acceptance/feeds.hurl (4)
35-45: Consider extracting the hardcoded delay to a variable.Line 38 uses a hardcoded
5sdelay, while line 27 uses the{{delay_ms}}variable. For consistency and flexibility, consider extracting this to a variable like{{refresh_delay_s}}.🔎 Proposed fix
# Get the changelog feed ONE MORE TIME GET {{proto}}://{{host}}/podcast/feed?hurl=true [Options] -delay: 5s # wait a bit so that it refreshes from origin +delay: {{refresh_delay_s}} # wait a bit so that it refreshes from origin HTTP 200 # expect OK response
13-13: Consider environment-specific duration thresholds.The hardcoded duration thresholds (< 1000ms for cold load, < 500ms for cached) are reasonable but could be flaky in CI environments or under variable network conditions. If test failures occur, consider making these thresholds configurable via variables.
Also applies to: 30-30, 41-41
9-60: Note: Tests are intentionally stateful and sequential.These tests validate cache behavior through a deliberate sequence (cold → stale → refresh → purge → miss). This means:
- Tests cannot run in isolation or be reordered
- A failure in one test may cause downstream test failures
- Parallel execution is not supported
This is an appropriate design for cache lifecycle validation, but consider documenting this dependency for future maintainers.
1-60: Consider adding error case coverage.The current tests focus on happy paths (200 responses). For more comprehensive coverage, consider adding tests for:
- Invalid or missing purge tokens (should return 403/401)
- Non-existent feed endpoints (404)
- Rate limiting scenarios
This is optional and can be deferred based on project priorities.
.github/workflows/_namespace.yml (1)
30-36: Consider parameterizing the deployment directory path.The hardcoded path
fly.io/cdn-2025-02-25appears in multiple workflow files (.github/workflows/_namespace.ymland.github/workflows/_github.ymlat line 34), creating a maintenance burden. The date in the path signals that updates may be needed over time. If you plan to support multiple backend deployments, extracting this to a workflow input, environment variable, or configuration file would improve flexibility and align with the PR's goal of supporting arbitrary backends.test/acceptance/periodic/region.hurl (1)
42-42: Consider documenting or parameterizing the hard-coded file size.The exact byte count assertion ensures file integrity but will fail if the MP3 file is ever updated. Consider adding a comment explaining this is intentional, or making the expected size configurable if the file may change.
justfile (1)
45-48: Minor indentation inconsistency.The
docker-bashtarget uses 2-space indentation while the file header indicatestabstop=4 shiftwidth=4. Other targets use 4-space indentation consistently.🔎 Proposed fix
# Open a shell in the docker container docker-bash: - @docker exec -it pipely.dev bash + @docker exec -it pipely.dev bashvarnish/vcl/nightly-backend.vcl (1)
24-31: Inconsistent indentation in vcl_synth.The
vcl_synthsubroutine mixes tabs (lines 25-27, 30) with spaces (lines 28-29). This should be consistent with the rest of the file which uses 2-space indentation.🔎 Proposed fix
sub vcl_synth { - # Reject non-GET/HEAD/PURGE requests - if (req.http.x-backend-nightly - && resp.status == 405) { + # Reject non-GET/HEAD/PURGE requests + if (req.http.x-backend-nightly + && resp.status == 405) { set resp.http.allow = "GET, HEAD, PURGE"; return(deliver); - } + } }test/acceptance/nightly.hurl (1)
46-55: Cache-status assertion missing for consistency.The GET request for the static asset validates several cache-related headers (
ageon line 54) but doesn't assert oncache-status, while the HEAD request on lines 56-66 does checkcache-status(line 66). For consistency with other test scenarios in this file (lines 10-15, 25-26, 37-42, 66), consider adding a cache-status assertion here as well.🔎 Suggested addition
header "via" matches /[vV]arnish/ # served via Varnish header "age" exists # cache age works +header "cache-status" contains "storage.memory" # cached in memoryNote: The Hurl syntax issue flagged in the previous review (response headers appearing before HTTP status) has been resolved in the current code—the HTTP 200 status line now correctly appears on line 48 before the assertions section.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.github/workflows/_github.yml.github/workflows/_namespace.ymlREADME.mdcontainer/justfiledagger/main.gojustfiletest/acceptance/admin.hurltest/acceptance/assets.hurltest/acceptance/feeds.hurltest/acceptance/health.hurltest/acceptance/homepage.hurltest/acceptance/nightly.hurltest/acceptance/periodic/region.hurltest/acceptance/production/assets.hurltest/acceptance/production/feed.hurltest/acceptance/production/feeds.hurltest/vtc/assets-backend.vtctest/vtc/fly/client-ip.vtctest/vtc/nightly-backend.vtcvarnish/vcl/backend-health-200.vclvarnish/vcl/backend-health-204.vclvarnish/vcl/default.vclvarnish/vcl/nightly-backend.vcl
💤 Files with no reviewable changes (3)
- test/acceptance/production/feeds.hurl
- test/acceptance/production/feed.hurl
- test/acceptance/production/assets.hurl
✅ Files skipped from review due to trivial changes (1)
- test/vtc/fly/client-ip.vtc
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/_github.yml
- test/vtc/assets-backend.vtc
- test/acceptance/health.hurl
- test/vtc/nightly-backend.vtc
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-30T13:45:33.339Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/feeds-backend.vcl:19-109
Timestamp: 2025-12-30T13:45:33.339Z
Learning: In the varnish/vcl/ directory, prefer explicit code repetition over complex regular expressions for maintainability and readability, even if it increases lines of code. This guideline applies to all .vcl files within varnish/vcl/ (and its subdirectories) to keep patterns straightforward and easier to review.
Applied to files:
varnish/vcl/nightly-backend.vclvarnish/vcl/backend-health-200.vclvarnish/vcl/backend-health-204.vclvarnish/vcl/default.vcl
📚 Learning: 2025-12-30T13:53:53.196Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/fly/client-ip.vcl:3-16
Timestamp: 2025-12-30T13:53:53.196Z
Learning: In the varnish/vcl/fly/client-ip.vcl file, the client IP determination logic only logs the selected IP via std.log and does not store it in a request header. This is intentional because the health-checker reads directly from log output rather than from request headers.
Applied to files:
varnish/vcl/nightly-backend.vclvarnish/vcl/default.vcl
📚 Learning: 2025-12-30T13:48:47.598Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/news-mp3.vcl:97-97
Timestamp: 2025-12-30T13:48:47.598Z
Learning: In varnish/vcl/news-mp3.vcl, the ASSETS_HOST environment variable should NOT have a fallback or default value. The code should fail if ASSETS_HOST is not set, following a fail-fast pattern for required configuration.
Applied to files:
varnish/vcl/nightly-backend.vclvarnish/vcl/default.vcl
🧬 Code graph analysis (1)
dagger/main.go (1)
dagger/proxy.go (2)
Proxy(8-13)NewProxy(15-27)
🔇 Additional comments (25)
test/acceptance/feeds.hurl (3)
1-8: LGTM!The basic RSS endpoint test correctly validates Cloudflare/Varnish serving and content type.
47-53: LGTM!The PURGE operation test correctly uses a templated purge token and validates the synthetic response from Varnish.
55-60: LGTM!The post-purge validation correctly confirms cache miss behavior after the PURGE operation.
.github/workflows/_namespace.yml (2)
38-41: LGTM! Production tests now run after deployment.The condition correctly ensures production acceptance tests only run after tag-based deployments complete, preventing test failures during the deployment window. This aligns perfectly with the PR objective to "reorganise the acceptance tests so production checks run after deploying the new version."
43-49: LGTM! Unconditional artifact upload improves debuggability.Using
always()ensures test acceptance reports are archived regardless of workflow outcome, which is essential for debugging failed tests or deployments.test/acceptance/periodic/region.hurl (2)
1-13: Good coverage for the news endpoint.The test assertions appropriately verify content-type, CORS, caching headers, debug information, and region-specific routing.
8-8: Clarify the origin backend inconsistency across the three endpoint tests.The first test expects
fly.devin thecache-statusheader for/news(line 8), while the second and third tests check for thecf-rayheader for/master/feedandcdn.changelog.com(lines 22, 36), indicating Cloudflare as the origin. These inconsistencies should be documented if intentional, or the assertions should be aligned if they represent the same routing behavior. Based on the VCL configuration,/uploads/news/routes through Cloudflare alongside other assets, which conflicts with thefly.devassertion on line 8.README.md (1)
21-21: LGTM!Roadmap item correctly marked as complete with the appropriate PR reference.
varnish/vcl/backend-health-204.vcl (1)
1-32: LGTM!Well-structured health probe with appropriate configuration. The timeout being less than the interval prevents probe overlap, and the 50% threshold provides reasonable fault tolerance.
varnish/vcl/backend-health-200.vcl (1)
1-6: LGTM!Good refactoring to explicitly name the probe
backend_health_200and add the.expected_response = 200property. This makes the configuration more explicit and distinguishes it clearly from the newbackend_health_204probe.dagger/main.go (4)
59-63: LGTM!The struct fields for
FeedsProxyandNightlyProxyare properly added alongside the existing proxy fields, maintaining consistency.
218-234: LGTM!The proxy initialization follows the established pattern with proper error handling. Each proxy is created via
NewProxyand assigned to the corresponding struct field.
250-273: LGTM!The Procfile and environment variable configuration are properly extended to include all four backends (app, assets, feeds, nightly) with consistent patterns.
102-109: Verify the default proxy configurations.The default values follow the expected
PORT:FQDN:HOSTformat. The ports (5000, 5010, 5020, 5030) are sequential and non-conflicting.varnish/vcl/nightly-backend.vcl (2)
3-21: LGTM!The routing logic correctly:
- Routes requests to the nightly backend based on host matching or the
/nightly_healthpath- Restricts methods to GET/HEAD/PURGE
- Rewrites the health check path and bypasses cache for fresh status
The use of
std.getenv("NIGHTLY_HOST")follows the fail-fast pattern for required configuration per existing learnings.
45-61: LGTM!The cache configuration is well-suited for a nightly/staging environment:
- Memory-only storage for faster access
- Short TTL (1 minute) ensures relatively fresh content
- Long grace (1 day) allows stale serving during backend issues
- Keep (7 days) reduces backend retransmissions
varnish/vcl/default.vcl (3)
13-21: LGTM!The app director correctly uses the renamed
backend_health_200probe while maintaining the elevatedfirst_byte_timeoutof 300s for MP3 uploads.
41-48: LGTM!The nightly director correctly uses
backend_health_204probe, which aligns with the nightly backend's/healthendpoint returning 204 No Content status.
53-54: LGTM!The include order is alphabetical and includes both health probe variants along with the new nightly-backend.vcl.
Also applies to: 62-62
test/acceptance/assets.hurl (2)
1-30: LGTM!The refactored asset tests are well-structured with consistent assertions covering content-type, CORS headers, Cloudflare/Varnish proxying, and cache behavior. The PURGE-then-GET flow correctly verifies cache invalidation.
66-119: Good coverage for MP3 range requests.The tests comprehensively cover:
- Full file download with exact byte count verification
- First chunk (bytes 0-1023)
- Middle chunk (bytes 1024-2047) with content-range verification
- Last chunk (bytes -1024) with proper offset verification
- Disk cache storage assertions
The hardcoded byte count (8575592) and content-range values are tightly coupled to the specific test file. If
changelog-news-140.mp3changes, these tests will fail. Consider documenting this dependency or verifying the file is stable.justfile (1)
65-79: No action required.The repository has no
production/*.hurlfiles to migrate. All acceptance tests are already organized undertest/acceptance/with no files existing in aproduction/directory. The justfile change correctly runs all available tests undertest/acceptance/, and the test scope is as intended.Likely an incorrect or invalid review comment.
container/justfile (3)
48-48: LGTM!The addition of
nightly_hostvariable aligns with the new nightly.changelog.com backend introduced in this PR and is correctly integrated into the test-acceptance-local configuration.
64-109: LGTM!The reorganized and expanded benchmark targets provide comprehensive coverage for testing app, assets, feed, and nightly backends across different environments (origin, TLS proxy, Fly.io, local). The consistent numbering scheme (1=origin, 2=TLS proxy, 3=Fly.io, 4=local) and the correct parameter passing to the
benchfunction make the structure clear and maintainable.
60-61: The Just conditional syntax for the host parameter is correct.The conditional expression
{{ if host != "" { "--host " + host } else { "" } }}follows Just's documented syntax for conditional expressions within interpolation. Whenhostis provided (e.g., frombench-assets-4-localwith "cdn.changelog.com"), it expands to--host cdn.changelog.com. Whenhostis empty (e.g., frombench-feed-4-local), it expands to an empty string. The syntax is valid and produces the expected output.
| new assets = dynamic.director( | ||
| ttl = 10s, | ||
| probe = backend_health_200, | ||
| host_header = std.getenv("BACKEND_ASSETS_FQDN"), | ||
| first_byte_timeout = 10s, | ||
| connect_timeout = 10s, | ||
| between_bytes_timeout = 60s | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the relevant section of default.vcl
find varnish -name "default.vcl" -type f | head -5Repository: thechangelog/pipely
Length of output: 86
🏁 Script executed:
# Check the context around lines 23-30 in default.vcl
if [ -f "varnish/vcl/default.vcl" ]; then
sed -n '15,40p' varnish/vcl/default.vcl
fiRepository: thechangelog/pipely
Length of output: 753
🏁 Script executed:
# Check news-mp3.vcl around line 97 to understand the learning context
if [ -f "varnish/vcl/news-mp3.vcl" ]; then
sed -n '90,105p' varnish/vcl/news-mp3.vcl
fiRepository: thechangelog/pipely
Length of output: 463
🏁 Script executed:
# Search for BACKEND_ASSETS_FQDN usage and how it's handled
rg "BACKEND_ASSETS_FQDN|ASSETS_HOST" varnish/ --context 3Repository: thechangelog/pipely
Length of output: 1660
🏁 Script executed:
# Search for any error handling or validation patterns around std.getenv calls
rg "std\.getenv" varnish/ -A 2 | head -50Repository: thechangelog/pipely
Length of output: 2762
🏁 Script executed:
# Check if there are any validation or required env var checks in the codebase
rg "require|fail|panic|throw" varnish/ --context 2 | grep -i "env\|getenv" | head -20Repository: thechangelog/pipely
Length of output: 180
Add validation to ensure BACKEND_ASSETS_FQDN environment variable is set.
The assets director uses std.getenv("BACKEND_ASSETS_FQDN") without validating that the variable is configured. When required environment variables are not set, the code should fail immediately rather than silently proceeding with an empty value. Add explicit validation to ensure BACKEND_ASSETS_FQDN is set at load time, following a fail-fast pattern for required configuration.
🤖 Prompt for AI Agents
In varnish/vcl/default.vcl around lines 23 to 30, the host_header uses
std.getenv("BACKEND_ASSETS_FQDN") directly so an unset env var yields an empty
value; capture the env var into a local variable at load time, check if it is
empty, and fail-fast (call panic with a clear message) if missing, then use that
validated variable for host_header in the assets director so the VCL refuses to
load when BACKEND_ASSETS_FQDN is not configured.
b20a271 to
477bcc5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
varnish/vcl/default.vcl (1)
23-30: Verify environment variable validation for BACKEND_ASSETS_FQDN.Similar to the past review comment,
std.getenv("BACKEND_ASSETS_FQDN")is used without explicit validation. While the Dagger configuration indagger/main.goprovides defaults, if the environment variable is unset at runtime, VCL will proceed with an empty value. Based on retrieved learnings, a fail-fast pattern is preferred for required configuration.
🧹 Nitpick comments (4)
.github/workflows/_github.yml (1)
26-32: Hardcoded deployment directory path.The path
fly.io/cdn-2025-02-25is hardcoded. If the directory name changes (e.g., for a new CDN version), this workflow will break silently. Consider parameterizing this or adding a comment explaining why this specific directory is used.test/acceptance/feeds.hurl (1)
142-150: Add a comment explaining why this test is skipped.The
skip: trueoption disables this test, but there's no explanation for why/feed/(with trailing slash) is skipped. Consider adding a comment to document the reason (e.g., known issue, redirect behavior, or intentional exclusion).test/acceptance/nightly.hurl (1)
45-66: Consider adding storage assertion to GET request for consistency.The HEAD test (line 66) asserts
cache-status contains "storage.memory", but the preceding GET test for the same asset (lines 45-55) does not include this assertion. Consider adding the storage check to the GET test as well for consistency, since the GET request primes the cache.🔎 Proposed fix
# GET a static asset GET {{proto}}://{{host}}/images/logo-night.png?hurl=true Host: {{nightly_host}} HTTP 200 # expect OK response [Asserts] header "content-type" contains "image/png" # expect PNG header "access-control-allow-origin" contains "*" # CORS header "fly-request-id" exists # served by Fly.io header "via" matches /[vV]arnish/ # served via Varnish header "age" exists # cache age works +header "cache-status" contains "storage=" # storage is setvarnish/vcl/nightly-backend.vcl (1)
24-31: Fix inconsistent indentation.The vcl_synth subroutine mixes tabs (lines 25-27, 30) and spaces (lines 28-29). Consider standardizing to one style for consistency.
🔎 Proposed fix
sub vcl_synth { - # Reject non-GET/HEAD/PURGE requests - if (req.http.x-backend-nightly - && resp.status == 405) { + # Reject non-GET/HEAD/PURGE requests + if (req.http.x-backend-nightly + && resp.status == 405) { set resp.http.allow = "GET, HEAD, PURGE"; return(deliver); - } + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.github/workflows/_github.yml.github/workflows/_namespace.ymlREADME.mdcontainer/justfiledagger/main.gojustfiletest/acceptance/admin.hurltest/acceptance/assets.hurltest/acceptance/feeds.hurltest/acceptance/health.hurltest/acceptance/homepage.hurltest/acceptance/nightly.hurltest/acceptance/production/assets.hurltest/acceptance/production/feed.hurltest/acceptance/production/feeds.hurltest/vtc/assets-backend.vtctest/vtc/fly/client-ip.vtctest/vtc/nightly-backend.vtcvarnish/vcl/backend-health-200.vclvarnish/vcl/backend-health-204.vclvarnish/vcl/default.vclvarnish/vcl/nightly-backend.vcl
💤 Files with no reviewable changes (3)
- test/acceptance/production/feed.hurl
- test/acceptance/production/assets.hurl
- test/acceptance/production/feeds.hurl
✅ Files skipped from review due to trivial changes (1)
- test/vtc/assets-backend.vtc
🚧 Files skipped from review as they are similar to previous changes (7)
- .github/workflows/_namespace.yml
- varnish/vcl/backend-health-204.vcl
- varnish/vcl/backend-health-200.vcl
- README.md
- justfile
- container/justfile
- test/vtc/fly/client-ip.vtc
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-30T13:48:47.598Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/news-mp3.vcl:97-97
Timestamp: 2025-12-30T13:48:47.598Z
Learning: In varnish/vcl/news-mp3.vcl, the ASSETS_HOST environment variable should NOT have a fallback or default value. The code should fail if ASSETS_HOST is not set, following a fail-fast pattern for required configuration.
Applied to files:
varnish/vcl/default.vclvarnish/vcl/nightly-backend.vcltest/acceptance/health.hurl
📚 Learning: 2025-12-30T13:45:33.339Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/feeds-backend.vcl:19-109
Timestamp: 2025-12-30T13:45:33.339Z
Learning: In the varnish/vcl/ directory, prefer explicit code repetition over complex regular expressions for maintainability and readability, even if it increases lines of code. This guideline applies to all .vcl files within varnish/vcl/ (and its subdirectories) to keep patterns straightforward and easier to review.
Applied to files:
varnish/vcl/default.vclvarnish/vcl/nightly-backend.vcl
📚 Learning: 2025-12-30T13:53:53.196Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/fly/client-ip.vcl:3-16
Timestamp: 2025-12-30T13:53:53.196Z
Learning: In the varnish/vcl/fly/client-ip.vcl file, the client IP determination logic only logs the selected IP via std.log and does not store it in a request header. This is intentional because the health-checker reads directly from log output rather than from request headers.
Applied to files:
varnish/vcl/default.vclvarnish/vcl/nightly-backend.vcl
🧬 Code graph analysis (1)
dagger/main.go (1)
dagger/proxy.go (2)
Proxy(8-13)NewProxy(15-27)
🔇 Additional comments (18)
test/acceptance/health.hurl (2)
16-22: LGTM!The assets_health test block is correctly structured with appropriate assertions for the assets backend routing.
32-38: LGTM!The nightly_health test correctly expects HTTP 204 to align with the new
backend_health_204probe, and assertions properly verify the nightly backend routing..github/workflows/_github.yml (1)
39-45: LGTM!Using
always()for artifact archiving ensures test reports are captured even when tests fail, which is valuable for debugging failed deployments.test/acceptance/feeds.hurl (1)
1-61: LGTM!Comprehensive cache lifecycle testing covering cold load, cached hits, stale refresh, PURGE operations, and post-purge cache miss behavior. The timing assertions and cache-status header checks provide thorough validation.
test/acceptance/assets.hurl (2)
1-31: LGTM!The asset test flow (GET → PURGE → GET-after-purge) properly validates the cache purge functionality with appropriate assertions for content type, CORS, and cache status.
77-119: LGTM!Thorough MP3 range request testing covering first, middle, and last chunks. The
content-rangeandbytes countassertions correctly validate partial content responses, andstorage.diskassertions confirm large file disk caching.dagger/main.go (3)
59-62: LGTM!The struct fields are properly ordered and documented, following the existing pattern for proxies.
218-234: LGTM!The proxy initialization follows the established pattern with proper error handling and assignment. The port allocation (5010 for assets, 5030 for nightly) maintains a logical progression.
250-256: LGTM!The procfile correctly includes all four TLS exterminator entries (app, assets, feeds, nightly) with proper string formatting.
varnish/vcl/default.vcl (2)
41-48: LGTM!The nightly dynamic director correctly uses
backend_health_204probe to support the nightly backend's HTTP 204 health response. The timeout configuration is consistent with other directors.
53-54: LGTM!The includes are properly organized: backend health probes (200 and 204) are included together, and the new
nightly-backend.vclis included in the correct alphabetical position.Also applies to: 62-62
test/acceptance/nightly.hurl (2)
1-26: LGTM!The cache lifecycle test for the nightly index page is well-structured, verifying cold cache performance (<1s) and cached performance (<100ms) with appropriate cache-status assertions.
28-43: LGTM!Good test coverage for a specific dated page with content body validation using regex matching.
varnish/vcl/nightly-backend.vcl (2)
3-22: LGTM!The vcl_recv subroutine correctly routes nightly requests based on host or health path, sets up backend hints using environment variables, and properly filters allowed HTTP methods. The health endpoint rewrite and pass-through is correct for bypassing cache on health checks.
45-61: LGTM!The caching strategy for nightly content is well-designed: memory-only storage for fast access, short TTL (1m) for freshness, generous grace (1d) for stale-while-revalidate, and extended keep (7d) to reduce backend retransmission. The comments explaining each setting are helpful.
test/vtc/nightly-backend.vtc (3)
1-31: LGTM! Well-structured mock backend and environment setup.The mock backend correctly defines all four endpoints (
/health,/images/logo-day.png,/night-theme.css,/images/star.png) needed by the test clients. Environment variables properly configure the nightly backend connection. The backend expects/health(line 7) while client c1 will request/nightly_health, which effectively tests the VCL's URL rewriting behavior for the health endpoint.
33-54: LGTM! Clean Varnish configuration for testing.The configuration correctly sets up a memory-backed cache, initializes a dynamic director without a health probe (appropriate for testing), and includes the necessary VCL modules. The PURGE method handling in
vcl_recvenables the method validation tests later in the file.
56-126: LGTM! Comprehensive test coverage with excellent validation.The seven client tests thoroughly validate:
- Health endpoint routing (c1 tests
/nightly_health→/healthrewriting without requiring a Host header)- HTTP method enforcement (c2-c5 verify 405 for POST/OPTIONS and 200 for PURGE/HEAD with correct
Allowheaders)- Asset delivery with cache validation (c6-c7 use
logexpectto confirm malloc memory storage)- CORS header delivery (c6-c7 verify
access-control-allow-origin: *)The corrected comment on line 113 ("Expectation for the PNG request") accurately describes the logexpect block, addressing the previous review feedback.
gerhard
force-pushed
the
add-nightly-backend
branch
from
477bcc5 to
eb05b09
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
justfile (1)
45-48: Inconsistent indentation indocker-bashtarget.The
docker-bashtarget uses 2-space indentation while other targets in this file use 4-space indentation (as configured in the vim modeline on line 1).🔎 Proposed fix
# Open a shell in the docker container docker-bash: - @docker exec -it pipely.dev bash + @docker exec -it pipely.dev bashtest/acceptance/nightly.hurl (1)
28-43: Hardcoded date may cause test brittleness.The test uses a hardcoded date
/2025/12/30/and body match for2025-12-30. This test will fail if the nightly page for that specific date is removed or becomes unavailable in the future.Consider using a more stable URL path (like the index page) or documenting that this test may need periodic updates.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.github/workflows/_github.yml.github/workflows/_namespace.ymlREADME.mdcontainer/justfiledagger/main.gojustfiletest/acceptance/admin.hurltest/acceptance/assets.hurltest/acceptance/feeds.hurltest/acceptance/health.hurltest/acceptance/homepage.hurltest/acceptance/nightly.hurltest/acceptance/production/assets.hurltest/acceptance/production/feed.hurltest/acceptance/production/feeds.hurltest/vtc/assets-backend.vtctest/vtc/fly/client-ip.vtctest/vtc/nightly-backend.vtcvarnish/vcl/backend-health-200.vclvarnish/vcl/backend-health-204.vclvarnish/vcl/default.vclvarnish/vcl/nightly-backend.vcl
💤 Files with no reviewable changes (3)
- test/acceptance/production/feed.hurl
- test/acceptance/production/feeds.hurl
- test/acceptance/production/assets.hurl
🚧 Files skipped from review as they are similar to previous changes (8)
- README.md
- varnish/vcl/default.vcl
- .github/workflows/_github.yml
- test/vtc/assets-backend.vtc
- varnish/vcl/nightly-backend.vcl
- varnish/vcl/backend-health-204.vcl
- test/vtc/nightly-backend.vtc
- test/vtc/fly/client-ip.vtc
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-30T13:45:33.339Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/feeds-backend.vcl:19-109
Timestamp: 2025-12-30T13:45:33.339Z
Learning: In the varnish/vcl/ directory, prefer explicit code repetition over complex regular expressions for maintainability and readability, even if it increases lines of code. This guideline applies to all .vcl files within varnish/vcl/ (and its subdirectories) to keep patterns straightforward and easier to review.
Applied to files:
varnish/vcl/backend-health-200.vcl
📚 Learning: 2025-12-30T13:48:47.598Z
Learnt from: gerhard
Repo: thechangelog/pipely PR: 44
File: varnish/vcl/news-mp3.vcl:97-97
Timestamp: 2025-12-30T13:48:47.598Z
Learning: In varnish/vcl/news-mp3.vcl, the ASSETS_HOST environment variable should NOT have a fallback or default value. The code should fail if ASSETS_HOST is not set, following a fail-fast pattern for required configuration.
Applied to files:
test/acceptance/health.hurl
🔇 Additional comments (19)
varnish/vcl/backend-health-200.vcl (1)
1-1: LGTM! Explicit probe naming and response code improve clarity.The rename to
backend_health_200and the addition of.expected_response = 200make the health probe's intent explicit, which is particularly valuable when multiple health probes with different expected responses exist in the codebase (e.g., thebackend_health_204probe variant). All references to the new probe name have been properly updated throughout the codebase.Also applies to: 6-6
.github/workflows/_namespace.yml (3)
30-36: Tag-gated deployment looks good.The condition correctly gates publish and deploy operations to version tag pushes, which is a standard practice for production releases. The sequence of publish → deploy → scale is logical.
Note: The directory path
fly.io/cdn-2025-02-25includes a hardcoded date, which may require updates in future releases if this represents a versioned deployment path.
38-41: Production acceptance tests appropriately sequenced after deployment.The tag-based gating ensures production acceptance tests run only after the deployment step, which aligns with the PR objective to "run production checks after deploying the new version to avoid pre-deployment failures." This is the correct approach for validating deployed changes.
43-49: Good practice: Unconditional artifact archiving.Using
always()ensures test acceptance reports are captured even when tests fail, which is valuable for debugging and post-mortem analysis. The glob patterntmp/test-acceptance-*will appropriately capture all acceptance test outputs.justfile (1)
56-79: LGTM!The renamed comments improve consistency (
Test acceptance local/Test acceptance production), and consolidating the test command to use onlytest/acceptance/*.hurlaligns with the reorganized test structure in this PR.test/acceptance/health.hurl (2)
16-22: LGTM!The assets health endpoint test correctly validates HTTP 200 response and the
origin=assetsassertion matches the comment.
32-38: LGTM!The nightly health endpoint test correctly validates HTTP 204 (No Content) response, which aligns with the PR objective to support backends that return HTTP 204. The
origin=nightlyassertion now correctly matches the comment.test/acceptance/assets.hurl (2)
1-31: LGTM!The new PURGE flow test for static assets correctly validates the GET → PURGE → GET sequence, ensuring cache invalidation works as expected. The assertions properly check for cache miss after purge.
77-119: LGTM!The MP3 range request tests are well-structured:
- First chunk (bytes 0-1023): validates 1024 bytes
- Middle chunk (bytes 1024-2047): validates content-range header and 1024 bytes
- Last chunk (bytes -1024): correctly validates the suffix range with proper content-range (
8574568-8575591/8575592= last 1024 bytes of 8575592 total)The byte arithmetic is correct:
8575592 - 1024 = 8574568for the start position.test/acceptance/nightly.hurl (1)
45-66: LGTM!The static asset tests correctly validate PNG content-type, CORS headers, and cache behavior. The HEAD request appropriately includes the
cache-statusstorage assertion.test/acceptance/feeds.hurl (2)
9-61: LGTM!The expanded changelog feed tests comprehensively validate the caching lifecycle:
- Cold cache load with TTL/grace/keep assertions
- Stale cache serving after delay exceeds TTL
- Cache refresh from origin
- PURGE invalidation
- Cache miss after purge
The use of configurable delay variables (
delay_ms,delay_s) makes the tests adaptable to different environments.
142-150: Clarify why/feed/endpoint test is skipped.The
skip: trueoption suppresses this test. If there's a known issue with this endpoint, consider adding a comment explaining why it's skipped or linking to a tracking issue.dagger/main.go (4)
59-62: LGTM!The new
FeedsProxyandNightlyProxyfields extend the Pipely struct consistently with the existingAppProxyandAssetsProxypattern.
102-109: LGTM!The new proxy parameters follow the established pattern with sensible defaults:
assetsProxy: port 5010, routes tochangelog.place→cdn.changelog.comnightlyProxy: port 5030, routes tochangelog-nightly-2023-10-10.fly.dev→nightly.changelog.comPort allocation (5000, 5010, 5020, 5030) is sequential and easy to follow.
218-234: LGTM!The proxy initialization follows the same error-handling pattern as the existing
AppProxysetup, correctly propagating errors fromNewProxy().
250-273: LGTM!The Procfile and environment variable configuration correctly wires all four proxies:
- Procfile entries for
feedsandnightlyTLS terminators- Backend environment variables (
BACKEND_FEEDS_*,BACKEND_NIGHTLY_*) follow the established naming patternNIGHTLY_HOSTmirrors the existingASSETS_HOSTpattern for host-based routingcontainer/justfile (3)
48-53: LGTM!The addition of
nightly_hostvariable correctly supports the new nightly backend tests. The test path update to includetest/acceptance/local/*.hurlaligns with the test reorganization.
60-61: LGTM!The bench function correctly adds optional host header support using justfile's conditional expression syntax. This enables Host-based routing for benchmarks that need to target specific virtual hosts (like assets and nightly via localhost).
66-109: LGTM!The benchmark targets are well-organized with a consistent numbering scheme:
*-1-origin: Direct backend*-2-tls-proxy: Through TLS terminator*-3-fly: Through Fly.io CDN*-4-local: Through local VarnishThe new nightly benchmarks follow this pattern, and the assets/local benchmark correctly passes the
cdn.changelog.comhost header.
Reorganise the acceptance tests & check production AFTER we deploy the new version (otherwise tests will fail). Add a 204 backend health probe so that we can support backends that return HTTP 204, otherwise the backend will be marked as "sick". Add bench-nightly-* so that we can see how it compares - see more details in the PR description. While at it, add assets benchmarks too, and use HTTP 1.1 until `oha` works with both `--http2 --host` flags. FWIW, if we want this to be useful for others, the code should be simpler for an arbitrary number of backends. Something that will need to improve as we approach v2.0. Signed-off-by: Gerhard Lazu <[email protected]>
gerhard
force-pushed
the
add-nightly-backend
branch
from
eb05b09 to
dd95d16
Compare
Nightly.Pipedream.2025-12-31.at.16.45.48.mp4
Reorganise the acceptance tests & check production AFTER we deploy the new version (otherwise tests will fail).
Add a 204 backend health probe so that we can support backends that return HTTP 204, otherwise the backend will be marked as "sick".
Add bench-nightly-* so that we can see how it compares - see more details in the PR description. While at it, add assets benchmarks too, and use HTTP 1.1 until
ohaworks with both--http2 --hostflags.FWIW, if we want this to be useful for others, the code should be simpler for an arbitrary number of backends. Something that will need to improve as we approach v2.0.
Related:
Summary by CodeRabbit
New Features
Tests
Benchmarks
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.