chore: upgrades dc.sh to use buildkit to store cache on gha #2224
Conversation
WalkthroughAdds test-detection and gated test/setup/teardown steps to the reusable GitHub Actions workflow; consolidates docker CLI into a Buildx/Bake-enabled script with BuildKit caching and unified Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(242,248,255)
participant WF as GitHub Workflow
participant Detect as tests_details (package.json)
participant Buildx as docker/setup-buildx-action
participant Prep as Prepare tests env
participant Test as Run tests
participant Teardown as Teardown tests env
participant Codecov as Coverage upload
end
WF->>Detect: read package.json -> outputs has_tests, requires_env_setup
Detect-->>WF: outputs(has_tests, requires_env_setup)
alt requires_env_setup == true
WF->>Buildx: setup buildx / ensure builder
Buildx-->>WF: builder ready
WF->>Prep: run test:ci-setup
Prep-->>WF: env ready
end
alt has_tests == true
WF->>Test: run test:ci (coverage)
Test-->>WF: results
WF->>Codecov: upload results & coverage
Codecov-->>WF: done
end
alt requires_env_setup == true
WF->>Teardown: run test:ci-teardown (always)
Teardown-->>WF: cleanup complete
end
sequenceDiagram
rect rgb(255,248,242)
participant User
participant DC as dc.sh
participant Parser as Flag/Compose Parser
participant Builder as ensure_builder
participant Bake as docker_buildx_bake / bake_cached
participant Compose as docker_compose
participant Docker as Docker Engine
end
User->>DC: ./dc.sh up [--no-db] [USE_DOCKER_BUILDKIT=1]
DC->>Parser: parse flags -> COMPOSE_FILES, REQUESTED_SERVICES
Parser-->>DC: parsed values
alt USE_DOCKER_BUILDKIT == 1
DC->>Builder: ensure_builder()
Builder->>Docker: create/verify builder
Builder-->>DC: ready
DC->>Bake: docker_buildx_bake / bake_cached (with cache)
Bake->>Docker: build images (BuildKit)
Bake-->>DC: images built
else
DC->>Docker: docker compose build (requested services)
end
DC->>Compose: docker_compose up (COMPOSE_FILES, services)
Compose->>Docker: start containers
Compose-->>DC: services running
DC-->>User: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-06-10T14:19:14.122ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
🔇 Additional comments (16)
Comment |
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![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
🧹 Nitpick comments (3)
packages/docker/script/dc.sh (3)
64-66: Directory context change is correct but path should be quoted.The subshell pattern
(cd $DC_FILES_DIR && ...)correctly isolates the directory context. However,$DC_FILES_DIRshould be quoted to handle paths with spaces:(cd "$DC_FILES_DIR" && ...).Apply this diff:
bake_cached() { - (cd $DC_FILES_DIR && docker_buildx_bake) + (cd "$DC_FILES_DIR" && docker_buildx_bake) }
68-71: Centralized compose helper is good, but apply it consistently across all commands.The
docker_composehelper correctly manages file paths and passthrough args. However, thedowncommand (line 131–132) bypasses this helper and directly callsdocker compose, whilebuildandupuse it. This inconsistency creates maintenance risk if compose invocation logic needs to change.Refactor the
downcommand to use thedocker_composehelper for consistency:down) - echo "Running: docker compose -p console down $*" - docker compose -p console down "$@" || { echo "Docker down failed"; exit 1; } + docker_compose -p console down || { echo "Docker down failed"; exit 1; } ;;This ensures all commands follow the same invocation pattern.
3-24: Help text examples are clear but could clarify the up/up:dev equivalence.The help examples correctly show
USE_DOCKER_BUILDKIT=1usage. However, lines 23 shows onlyup:devin the BuildKit example. Since lines 134 treatsupandup:devas equivalent, clarify in the help that both work or simplify to justup.Update line 23 to show both forms:
- echo " USE_DOCKER_BUILDKIT=1 $COMMAND_PREFIX up:dev <docker compose args> # Builds Docker images using buildkit and start them" + echo " USE_DOCKER_BUILDKIT=1 $COMMAND_PREFIX up <docker compose args> # Build images using buildkit and start services"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/reusable-validate-app.yml(2 hunks)packages/docker/script/dc.sh(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: Validate local packages
🔇 Additional comments (6)
.github/workflows/reusable-validate-app.yml (3)
38-44: Verify test detection robustness.The preflight check detects
test:covfor test presence, but consider documenting whether apps must use exactly this script name or if there's flexibility for other test runners. If standardization is required across apps, ensure it's enforced elsewhere.
49-55: Environment variable naming and cache scope design look sound.The
USE_DOCKER_BUILDKIT=1andDOCKER_BUILDKIT_CACHE_SCOPEpattern with per-app scoping (${{ github.workflow }}-${{ inputs.app }}) correctly isolates caches and avoids conflicts between parallel builds of different apps.
61-63: Teardown lifecycle design is correct.Using
always()ensures cleanup runs even if earlier steps fail, which is the right approach for environment teardown. However, verify that if "Prepare tests environment" fails, leaving a partially-initialized state,test:ci-teardowncan safely handle and clean it up.packages/docker/script/dc.sh (3)
26-37: Array helper function is well-implemented.The
remove_array_item_by_valuefunction correctly uses bash namerefs to mutate the original array in place. Implementation is solid and safely removes the composed file when--no-dbis specified.
39-46: BuildKit availability check is appropriate.The
ensure_builderfunction correctly verifiesdocker buildxavailability and either creates or switches toci-builder. The error exit on missing buildx is the right approach to fail fast with a clear message.
121-139: Command dispatch logic correctly gates BuildKit usage but verify service ordering.The conditional BuildKit execution in
buildandupcases correctly checksUSE_BUILDKITbefore callingbake_cached. Ensure thatdocker composecorrectly handles service names placed after options in line 138. The orderdocker compose -f ... up --renew-anon-volumes service1 service2should be valid, but verify withdocker compose --helpif this deviates from your testing.
stalniy
force-pushed
the
chore/api-build
branch
from
8b4d626 to
f73389a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/docker/script/dc.sh (1)
48-62: ** GHA cache configuration is environment-specific but lacks context detection.**This replicates the prior concern: the hardcoded
type=ghacache (lines 58–59) only works within GitHub Actions. The function will fail or produce errors when invoked locally or in non-GHA CI systems. The past review requested either context detection or clear fallback behavior.To resolve, detect the
GITHUB_ACTIONSenvironment variable and conditionally apply cache settings:docker_buildx_bake() { ensure_builder # Build only requested services if any; otherwise build all targets from compose local targets=() if [[ ${#REQUESTED_SERVICES[@]} -gt 0 ]]; then targets=("${REQUESTED_SERVICES[@]}") fi docker buildx bake \ $(printf -- " --file %q" "${COMPOSE_FILES[@]}") \ + $([ "$GITHUB_ACTIONS" = "true" ] && printf -- " --set *.cache-from=type=gha,scope=%s --set *.cache-to=type=gha,mode=max,scope=%s" "$BUILDKIT_CACHE_SCOPE" "$BUILDKIT_CACHE_SCOPE") \ --load \ "${targets[@]}" }Alternatively, use a local buildkit cache backend when outside GHA and document the limitation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/reusable-validate-app.yml(2 hunks)packages/docker/script/dc.sh(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/reusable-validate-app.yml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: Validate local packages
stalniy
force-pushed
the
chore/api-build
branch
3 times, most recently
from
d6f76ed to
75fa85d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/docker/script/dc.sh (2)
73-73: Unquoted variable path breaks on spaces or special characters — issue persists from past review.This was flagged previously and marked as addressed, but the unquoted
$DC_FILES_DIRin the subshell remains. If the path contains spaces or special characters, the command will fail.bake_cached() { - (cd $DC_FILES_DIR && docker_buildx_bake) + (cd "$DC_FILES_DIR" && docker_buildx_bake) }
109-109: KNOWN_SERVICES extraction silently fails on invalid compose files — issue persists from past review.This issue was flagged in the previous review and remains unresolved. The stderr redirection to
/dev/nullmasks failures from missing or malformed compose files. Ifdocker compose configfails,KNOWN_SERVICESbecomes empty, causing the service matching logic (lines 119–122) to silently skip all requested services, and downstream builds/ups proceed without the specified services.Additionally,
COMPOSE_FILESpaths are never validated before use, making this scenario realistic.The fix requires validating compose files before use and not discarding
docker composeerrors. Useset -o pipefailor capture the docker exit code explicitly:+# Validate compose files exist +for f in "${COMPOSE_FILES[@]}"; do + if [[ ! -f "$f" ]]; then + echo "Error: Compose file not found: $f" >&2 + exit 1 + fi +done + +# Enable pipefail to catch docker compose failures +set -o pipefail + -KNOWN_SERVICES=$(docker compose $(printf -- " -f %q" "${COMPOSE_FILES[@]}") config --services 2> /dev/null | tr '\n' ' ') +KNOWN_SERVICES=$(docker compose $(printf -- " -f %q" "${COMPOSE_FILES[@]}") config --services | tr '\n' ' ') || { echo "Error: Failed to retrieve services from Docker Compose" >&2; exit 1; }
🧹 Nitpick comments (6)
.github/workflows/reusable-validate-app.yml (1)
44-50: Consider adding explicit error handling for test:ci-setup.The "Prepare tests environment" step uses
--if-present, so it gracefully skips if the script doesn't exist. However, whenrequires_env_setupis true, the expectation is that the script will exist. If it's missing, the step silently succeeds, which could mask configuration errors. Consider adding a validation check or at least a clear error message if the script is expected but missing.packages/docker/script/dc.sh (5)
63-69: Debug output echo doesn't match executed command due to variable transformation.Line 63 echoes the command with unmodified
BUILDKIT_CACHE_SCOPE, but the actual execution (lines 66–67) replaces spaces with dashes (${BUILDKIT_CACHE_SCOPE// /-}). This makes the debug output misleading—developers see a different command than what actually ran, complicating troubleshooting.Either update the echo to reflect the transformation or remove it if not needed for debugging.
- echo "docker buildx bake $(printf -- " --file %q" "${COMPOSE_FILES[@]}") --set *.cache-from="type=gha,scope=${BUILDKIT_CACHE_SCOPE}" --set *.cache-to="type=gha,mode=max,scope=${BUILDKIT_CACHE_SCOPE}" --load ${targets[@]}" + echo "docker buildx bake $(printf -- " --file %q" "${COMPOSE_FILES[@]}") --set *.cache-from=\"type=gha,scope=${BUILDKIT_CACHE_SCOPE// /-}\" --set *.cache-to=\"type=gha,mode=max,scope=${BUILDKIT_CACHE_SCOPE// /-}\" --load ${targets[@]}"
90-94: COMMAND_PREFIX depends on undefined npm_command variable.The
npm_commandvariable is never defined in this script; it's implicitly expected to be set by npm when the script runs as an npm script. This is fragile—ifnpm_commandis unset,COMMAND_PREFIXsilently defaults to"./dc.sh", which may not be the intended behavior. Consider making this more explicit or adding a comment documenting this dependency.+# npm_command is set by npm when this script is invoked as an npm script if [ "$npm_command" = "run-script" ]; then COMMAND_PREFIX="npm run dc --" else COMMAND_PREFIX="./dc.sh" fi
111-125: Argument parsing and shifting logic is correct but could be clearer.The loop correctly processes
--no-db, removes the DB service file, matches known services, and shifts them out of$@, leaving only docker compose flags for downstream use. However, the pattern of callingshiftduring loop iteration over$@is not immediately obvious. Consider adding a brief comment to clarify the intent.for arg in "$@"; do case $arg in --no-db) + # Remove DB service and shift it out of $@ remove_array_item_by_value COMPOSE_FILES "${DC_FILES_DIR}docker-compose.prod-with-db.yml" shift echo "Excluding DB services from Docker Compose." ;; *) if [[ " $KNOWN_SERVICES " == *" $arg "* ]]; then + # Known service: add to REQUESTED_SERVICES and shift out of $@ REQUESTED_SERVICES+=("$arg") shift fi ;; esac done
136-139: Inconsistent command handling: down doesn't use docker_compose wrapper or COMPOSE_FILES.The
downcommand runsdocker composedirectly without using thedocker_composewrapper function and without referencingCOMPOSE_FILES. This is inconsistent with thebuildandupcommands, which both use the wrapper and compose files. For correctness and consistency,downshould also use the wrapper and the same compose files so docker knows exactly what to remove.down) - echo "Running: docker compose -p console down $*" - docker compose -p console down $@ || { echo "Docker down failed"; exit 1; } + docker_compose down $@ || { echo "Docker down failed"; exit 1; }Note: Update the
docker_composefunction to handle the-p consoleflag, or pass it explicitly here if the function doesn't add it automatically.
40-40: Consider improving error messaging in ensure_builder function.When Docker Buildx is not available, the error message is printed but the exit is not explicit about the error context. The message goes to stdout instead of stderr. For better debugging, consider redirecting the error message to stderr.
ensure_builder() { - docker buildx version >/dev/null 2>&1 || { echo "Docker Buildx not available"; exit 1; } + docker buildx version >/dev/null 2>&1 || { echo "Docker Buildx not available" >&2; exit 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/reusable-validate-app.yml(2 hunks)packages/docker/script/dc.sh(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: Validate local packages
🔇 Additional comments (8)
.github/workflows/reusable-validate-app.yml (5)
26-31: Clarify intent of gating static analysis to pull_request events only.The static code analysis step is now conditionally run only on pull_request events. This differs from the prior behavior where it may have run on all events. Confirm this is intentional—it could mean push-to-main no longer validates linting, which might allow issues to slip through.
33-39: Verify test detection logic accounts for all edge cases.The workflow detects
has_testsby checking for the presence of atest:covscript in package.json. However, line 54 runsnpm run test:covunconditionally without--if-present, so if the detection fails or package.json changes between detection and execution, the step will fail. Consider using--if-presentas a defensive measure, or verify that the detection logic is bulletproof.Additionally, confirm that
requires_env_setupdetection via thetest:ci-setupscript is consistent across all apps that require Docker setup.
41-50: Confirm BuildKit cache scope isolation strategy.The cache scope is set to
${{ github.workflow }}-${{ inputs.app }}, which should provide adequate isolation for multi-app repos. However, verify that:
- The cache scope is compatible with the dc.sh helper script's BuildKit configuration.
- The cache doesn't create stale/orphaned entries across different branches or workflow runs.
Also note that BuildKit environment variable setup (lines 47–48) assumes downstream scripts (like dc.sh) will consume these variables. Verify the integration with the docker helper script that this workflow triggers.
56-58: LGTM: Teardown properly usesalways()for guaranteed cleanup.The use of
always()ensures the teardown step runs even if prior steps fail, which is the correct pattern for resource cleanup. The additional gating onrequires_env_setupprevents unnecessary cleanup steps when no setup was performed.
60-67: Verify codecov integration still works with dynamic test detection.Both codecov steps now depend on
steps.tests_details.outputs.has_testsinstead of a run output. Confirm that:
- Codecov gracefully handles the case where no test artifacts (junit.xml or coverage files) exist.
- Apps without tests don't trigger false negatives in the CI/CD pipeline.
Also note that the "Upload test results" step uses
!cancelled()while "Upload Test Coverage" only uses the has_tests condition. Confirm this asymmetry is intentional.Also applies to: 69-76
packages/docker/script/dc.sh (3)
76-79: The docker_compose wrapper function is well-designed.This function correctly echoes the command for debugging and passes through arguments flexibly. The -p console flag is handled by callers (lines 138, 144), which provides good separation of concerns.
26-37: Helper function remove_array_item_by_value is well-implemented.The use of nameref (
local -n) for array reference passing is the correct bash idiom for modifying arrays in-place. The filter logic is straightforward and effective.
3-24: Help text is clear and well-maintained.The updated examples properly document the new
USE_DOCKER_BUILDKITusage and theup:devcommand. Good alignment with the actual implementation.
stalniy
force-pushed
the
chore/api-build
branch
3 times, most recently
from
adfa989 to
6f21d4b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/docker/script/dc.sh (3)
48-70:⚠️ CRITICAL: Unquoted array expansion in loop will break on service names with spaces.Line 56 uses
for target in $REQUESTED_SERVICES; dowhich splits on whitespace. If a service name contains spaces, it will be incorrectly split into multiple targets.This issue was flagged in past review but still appears in the current code. Fix by using proper array expansion:
- for target in $REQUESTED_SERVICES; do + for target in "${REQUESTED_SERVICES[@]}"; doEnsure
REQUESTED_SERVICESis populated as a bash array (which it is on line 101 and line 120), so element preservation works correctly.
72-74:⚠️ CRITICAL: Unquoted variable path will break on directories with spaces or special characters.Line 73 uses
cd $DC_FILES_DIRwithout quotes in a subshell. IfDC_FILES_DIRcontains spaces or special characters, the command will fail silently or misbehave. The function was recently introduced in commit adfa989 with this vulnerability already present.Apply this fix:
- (cd $DC_FILES_DIR && docker_buildx_bake) + (cd "$DC_FILES_DIR" && docker_buildx_bake)
99-109: MAJOR: Silent failure in KNOWN_SERVICES extraction masks configuration errors — CONFIRMED.Verification confirms the issue persists in line 109. The
stderrredirection (2> /dev/null) hides failures fromdocker compose config, and the absence ofset -o pipefailmeans the pipeline exit status is lost—onlytr's exit code is checked. If docker compose fails,KNOWN_SERVICESbecomes an empty string, causing silent service matching failures at line 119 with no user-facing error.All four referenced compose files exist at correct paths, but there is no validation before use. Configuration errors remain invisible to users.
Suggested fix:
- Validate each
COMPOSE_FILESpath exists before invoking docker compose- Do not discard docker compose stderr; let errors surface
- Enable
set -o pipefailor capture docker's exit code explicitly to fail fast-KNOWN_SERVICES=$(docker compose $(printf -- " -f %q" "${COMPOSE_FILES[@]}") config --services 2> /dev/null | tr '\n' ' ') +for file in "${COMPOSE_FILES[@]}"; do + [[ -f "$file" ]] || { echo "Error: Compose file not found: $file"; exit 1; } +done +KNOWN_SERVICES=$(docker compose $(printf -- " -f %q" "${COMPOSE_FILES[@]}") config --services | tr '\n' ' ') || { echo "Error: Failed to retrieve services from docker compose"; exit 1; }Alternatively, add
set -o pipefailat line 2 (after shebang) to auto-fail pipelines on docker compose errors.
🧹 Nitpick comments (1)
packages/docker/script/dc.sh (1)
111-125: Service targeting and --no-db handling looks sound, but service name matching is string-based.The argument parsing correctly identifies
--no-db, removes the DB compose file, and matches requested services againstKNOWN_SERVICES. The user-facing feedback is helpful.One minor consideration: Line 119 uses string matching (
" $KNOWN_SERVICES " == *" $arg "*) which assumes service names don't contain spaces. If a service name could contain spaces, this approach would fail. However, service names in Docker Compose typically don't include spaces, so this is unlikely to be an issue in practice.If robustness against edge-case service names is needed, consider parsing
KNOWN_SERVICESinto an array instead of a space-separated string to enable reliable matching of names with embedded spaces.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/reusable-validate-app.yml(2 hunks)packages/docker/script/dc.sh(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Validate local packages
🔇 Additional comments (7)
.github/workflows/reusable-validate-app.yml (4)
33-39: Test detection via package.json scripts is clean, but verify edge cases.The step correctly checks for
test:covandtest:ci-setupscripts in the app's package.json. Ensure that:
- Apps without either script still proceed (both variables default to 'false')
- The node -p syntax is resilient to missing/malformed package.json files
Consider adding error handling to fail fast if package.json is invalid.
44-50: Environment variables passed to test:ci-setup—ensure downstream scripts consume them correctly.The workflow sets
USE_DOCKER_BUILDKIT=1andDOCKER_BUILDKIT_CACHE_SCOPEand passes them tonpm run test:ci-setup. Verify that:
- The npm script (or any Docker commands it invokes) correctly reads these environment variables
DOCKER_BUILDKIT_CACHE_SCOPEfollows Docker naming conventions (alphanumeric, hyphens only; spaces converted to hyphens per the dc.sh implementation)Spot-check by tracing the flow: workflow → npm script → dc.sh (line 100).
52-54: Approve test and upload logic—conditions are correctly aligned.The test execution and Codecov uploads are properly gated on
steps.tests_details.outputs.has_tests. The teardown step correctly usesalways()to ensure environment cleanup even if tests fail, while the upload steps use!cancelled()and conditional checks to avoid running on skip/cancellation.Also applies to: 60-70
26-31: No issues found—ESLint --quiet flag syntax and support verified.The --quiet flag is documented in ESLint CLI as a standard way to suppress warnings and report only errors, and the syntax on line 31 is correct. The
--separator properly isolates the eslint flag from npm arguments, which is the standard approach for passing flags to npm scripts.packages/docker/script/dc.sh (3)
127-135: Build command routing is sound with proper error handling.The build command correctly isolates to the build compose file, conditionally uses BuildKit when
USE_BUILDKIT=1, and falls back to standard docker compose build. Service targeting viaREQUESTED_SERVICESis preserved and passed through correctly. Error messages are clear.
136-139: Down command doesn't respect COMPOSE_FILES or --no-db option.The down command hardcodes
docker compose -p console downwithout using theCOMPOSE_FILESarray or thedocker_composewrapper. This means--no-dbis processed during argument parsing (removing the DB compose file fromCOMPOSE_FILES) but then ignored by the down command.Depending on intent, consider whether down should:
- Respect the same COMPOSE_FILES as up (i.e., exclude DB when
--no-dbis used)- Or intentionally always bring down all services regardless of --no-db
If the former, apply this fix:
- echo "Running: docker compose -p console down $*" - docker compose -p console down $@ || { echo "Docker down failed"; exit 1; } + docker_compose -p console down $@ || { echo "Docker down failed"; exit 1; }Verify the intended behavior: should
--no-dbaffect the down operation or only up/build?
140-145: Up command correctly routes through BuildKit when enabled and respects service targeting.The up command properly handles the BuildKit flow (build then run), uses the
docker_composewrapper to respectCOMPOSE_FILESand--no-dboptions, and preservesREQUESTED_SERVICES. The--renew-anon-volumesflag is a good dev-time default. Error handling is consistent with other commands.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/docker/script/dc.sh (2)
76-77: Unquoted variable in subshell will break on paths with spaces.Line 76 uses unquoted
$DC_FILES_DIRin a subshell. If the path contains spaces or special characters, thecdcommand will fail.bake_cached() { - (cd $DC_FILES_DIR && docker_buildx_bake) + (cd "$DC_FILES_DIR" && docker_buildx_bake) }
112-112: KNOWN_SERVICES extraction needs error handling.Line 112 extracts services without validating that
docker compose configsucceeded. If any compose file is missing or malformed,KNOWN_SERVICESbecomes empty, causing service matching at line 122 to silently skip, and builds proceed without user-requested services.Validate compose files exist before extraction and fail fast if
docker compose configreturns an error:+# Validate compose files exist +for file in "${COMPOSE_FILES[@]}"; do + if [[ ! -f "$file" ]]; then + echo "Error: Compose file not found: $file" >&2 + exit 1 + fi +done + -KNOWN_SERVICES=$(docker compose $(printf -- " -f %q" "${COMPOSE_FILES[@]}") config --services | tr '\n' ' ') +KNOWN_SERVICES=$(docker compose $(printf -- " -f %q" "${COMPOSE_FILES[@]}") config --services) || { echo "Error: Failed to retrieve services from Docker Compose"; exit 1; } +KNOWN_SERVICES=$(echo "$KNOWN_SERVICES" | tr '\n' ' ')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/reusable-validate-app.yml(2 hunks)packages/docker/script/dc.sh(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
🪛 GitHub Actions: GitHub Actions Linting
.github/workflows/reusable-validate-app.yml
[error] 49-49: YAML syntax error in reusable workflow: could not parse as YAML at line 49 ('BUILDX_BAKE_ENTITLEMENTS_FS=0'). Expected ':' after key.
🪛 YAMLlint (1.37.1)
.github/workflows/reusable-validate-app.yml
[error] 50-50: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Validate local packages
🔇 Additional comments (2)
.github/workflows/reusable-validate-app.yml (1)
33-60: Workflow structure looks sound after syntax fix.The refactored test detection, environment preparation, and conditional teardown logic is well-designed. Test detection derives
has_testsandrequires_env_setupfrom package.json, and the subsequent steps properly gate BuildKit setup, environment prep, test execution, and teardown based on these outputs. The transition fromrun_tests.has_teststotests_details.has_testsis consistently applied.packages/docker/script/dc.sh (1)
57-62: Array expansion for REQUESTED_SERVICES is correct.The loop correctly uses
"${REQUESTED_SERVICES[@]}"to preserve service names with spaces, fixing the past issue flagged at this location.
stalniy
force-pushed
the
chore/api-build
branch
7 times, most recently
from
5b98267 to
1611f78
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/docker/script/dc.sh (2)
73-75: CRITICAL: Unquoted variable path breaks with spaces.Line 74 uses an unquoted
$DC_FILES_DIRvariable in the subshell, which will fail if the directory path contains spaces or special characters. This issue was previously flagged but remains unresolved in the current code.Apply this diff to quote the variable:
bake_cached() { - (cd $DC_FILES_DIR && docker_buildx_bake) + (cd "$DC_FILES_DIR" && docker_buildx_bake) }
108-114: Validate COMPOSE_FILES paths before use to catch configuration errors early.The script hardcodes COMPOSE_FILES paths but does not validate their existence before invoking
docker compose config --servicesat line 114. If any file is missing or malformed, KNOWN_SERVICES will silently become empty, causing subsequent service matching to skip without warning. Later, service selection logic at line 124 will not add any requested services.Add path validation before line 114:
COMPOSE_FILES=( "${DC_FILES_DIR}docker-compose.build.yml" "${DC_FILES_DIR}docker-compose.prod.yml" "${DC_FILES_DIR}docker-compose.prod-with-db.yml" "${DC_FILES_DIR}docker-compose.dev.yml" ) + +# Validate that all compose files exist +for file in "${COMPOSE_FILES[@]}"; do + if [[ ! -f "$file" ]]; then + echo "Error: Required Docker Compose file not found: $file" >&2 + exit 1 + fi +done + KNOWN_SERVICES=$(docker compose $(printf -- " -f %q" "${COMPOSE_FILES[@]}") config --services | tr '\n' ' ')Additionally, capture docker's exit status to fail early if
docker compose configfails:+docker compose $(printf -- " -f %q" "${COMPOSE_FILES[@]}") config --services > /tmp/dc_services.txt || { + echo "Error: Failed to retrieve available services from Docker Compose" >&2 + exit 1 +} +KNOWN_SERVICES=$(cat /tmp/dc_services.txt | tr '\n' ' ')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/reusable-validate-app.yml(2 hunks)apps/api/package.json(1 hunks)apps/notifications/package.json(1 hunks)packages/docker/Dockerfile.nextjs(1 hunks)packages/docker/Dockerfile.node(2 hunks)packages/docker/script/dc.sh(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/docker/Dockerfile.node
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate (apps/stats-web) / validate-unsafe
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: Validate local packages
🔇 Additional comments (8)
packages/docker/Dockerfile.nextjs (1)
43-43: BuildKit cache mount syntax is correct.The addition of
--mount=type=cachefor npm dependencies and removal ofnpm cache clean --forcealigns with BuildKit caching best practices. The cache mount point (/root/.npm) is appropriate for npm operations.packages/docker/script/dc.sh (1)
26-46: Helper functions follow good practices with proper quoting.The new helper functions (
remove_array_item_by_value,ensure_builder) demonstrate proper bash practices with quoted array handling and defensive existence checks. Theensure_builderfunction correctly uses conditional logic to create or select the ci-builder buildx instance.apps/api/package.json (1)
27-27: Verify thatdc up(without:dev) starts the same services asdc up:devdid.Line 27 changes the test:ci-setup script from
dc up:dev -d db mock-oauth2-server provider-proxytodc up -d db mock-oauth2-server provider-proxy. While dc.sh maps bothup:devandupto the same handler (line 147), confirm that removing the:devsuffix does not alter which Docker Compose files are loaded or service startup behavior in CI.Verify the following in your local environment or CI logs:
- Run
dc up -d dband confirm all expected services start- Run
dc up:dev -d dband confirm the output is identical- Check that the service filtering at line 124–126 of dc.sh correctly identifies and starts only the requested services
Alternatively, you can inspect the dc.sh implementation to confirm both commands follow the same code path.
apps/notifications/package.json (1)
29-29: Fix: Changes brokendc up:dbcommand to standarddc up -d db.The previous test:ci-setup script used
dc up:db, which is not a recognized dc.sh command (line 154 of dc.sh would treat it as unknown). This change todc up -d dbfixes the command to use the supportedupcommand with explicit service specification and detached mode, aligning with the pattern introduced in apps/api/package.json..github/workflows/reusable-validate-app.yml (4)
33-39: Introspection-based test gating is well-implemented.The workflow correctly derives whether an app has tests and requires environment setup by inspecting the package.json scripts. The step outputs (
has_testsandrequires_env_setup) are properly set and used to gate subsequent Docker setup and test execution steps, reducing unnecessary CI overhead for apps without tests.
60-62: Gated test execution ensures tests only run when present.The "Run tests" step properly gates on
has_testsand uses the correct command. This avoids unnecessary CI failures in apps without test suites.
64-66: Proper teardown guarantees cleanup even when tests fail.The "Teardown tests environment" step uses
always()condition combined with therequires_env_setupgate, ensuring Docker services are cleaned up even if tests fail, while avoiding unnecessary cleanup if no environment was set up. This is a best practice for CI workflows.
50-58: Verification complete: env vars correctly align with dc.sh expectations.All concerns have been verified:
✓ Env vars passed correctly: The workflow "Prepare tests environment" step passes
DOCKER_BUILDKIT_CACHE_SCOPEandDOCKER_BUILDKIT_CACHE_FALLBACK_SCOPEtonpm run test:ci-setup.✓ dc.sh reads variables at lines 100–105:
- Line 101: reads
DOCKER_BUILDKIT_CACHE_SCOPE- Line 104: reads
DOCKER_BUILDKIT_CACHE_FALLBACK_SCOPE✓ Cache scopes properly normalized: Lines 102 and 105 replace spaces with hyphens, confirmed working correctly with test values.
No issues found. The implementation correctly handles the BuildKit cache scope environment variables as intended.
stalniy
force-pushed
the
chore/api-build
branch
from
1611f78 to
794af59
Compare
Why
To speed up env setup by storing docker cache in gha
Summary by CodeRabbit