Conversation
📝 WalkthroughWalkthroughAdds Docker support: a multi-stage Dockerfile building two Go binaries, a docker-entrypoint.sh to orchestrate sync+backtest runs, a GitHub Actions workflow to build and push images to ghcr.io on tagged releases, and Docker usage documentation in README (quick start and local build). Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (88.99%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #325 +/- ##
==========================================
+ Coverage 88.89% 88.99% +0.10%
==========================================
Files 205 205
Lines 5761 5761
==========================================
+ Hits 5121 5127 +6
+ Misses 567 563 -4
+ Partials 73 71 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
.github/workflows/docker.yml (1)
31-33: Pre-release tags will overwrite:latest.Any
v*push — includingv2.0.0-betaorv1.0.0-rc1— unconditionally updates thelatesttag. Consider gating thelatesttag on stable semver tags only (no pre-release suffix).💡 Example using `docker/metadata-action` to control tag promotion
- name: Docker metadata id: meta uses: docker/metadata-action@v5 with: images: ghcr.io/cinar/indicator tags: | type=semver,pattern={{version}} type=semver,pattern={{major}}.{{minor}},enable=${{ !startsWith(github.ref, 'refs/tags/v0.') }} type=raw,value=latest,enable=${{ !contains(github.ref_name, '-') }} - name: Build and push uses: docker/build-push-action@v6 with: context: . push: true tags: ${{ steps.meta.outputs.tags }} labels: ${{ steps.meta.outputs.labels }} cache-from: type=gha cache-to: type=gha,mode=max🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/docker.yml around lines 31 - 33, The workflow currently always pushes ghcr.io/cinar/indicator:latest (via the tags block referencing github.ref_name), which lets pre-release tags overwrite :latest; change the tagging step to gate the latest tag to stable semver only — e.g. replace the static tags block with a docker/metadata-action invocation (docker/metadata-action@v5) to generate tags and only enable the raw latest tag when the tag name has no pre-release suffix (use a condition like enable=${{ !contains(github.ref_name, '-') }}), then pass the generated tags (steps.meta.outputs.tags) into docker/build-push-action so pre-release refs (e.g. v2.0.0-beta) do not set :latest.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/docker.yml:
- Around line 27-35: The workflow uses docker/build-push-action@v5 which lacks
support required for GitHub Cache API v2; update the action reference to
docker/build-push-action@v6 in the job that includes the uses: line, and ensure
your workflow uses a recent buildx/BuildKit (BuildKit ≥ 0.20.0 and buildx ≥
0.21.0) via the action inputs or preceding setup-steps so the
cache-from/type=gha and cache-to/type=gha settings work with v2.
In `@docker-entrypoint.sh`:
- Line 80: The current echo prints the full API_KEY (echo " API Key:
***${API_KEY:-none}") leaking secrets; change the echo to avoid printing the raw
value by replacing it with a masked or redacted representation: compute a masked
string from API_KEY (e.g., show "<redacted>" or build a mask that keeps only the
last 4 characters and replaces the rest with asterisks) and use that masked
variable in the echo, referencing the API_KEY variable and the echo statement so
the full secret is never emitted to stdout.
- Around line 48-51: The --assets case in docker-entrypoint.sh currently sets
ASSETS="$2" and shift 2 which only captures one token; change the --assets case
to collect all subsequent non-flag tokens into ASSETS (e.g., loop reading "$2"
while it does not start with '-' and append to ASSETS), advancing shifts for
each consumed token, then continue the outer argument parsing; update the case
label handling for --assets and ensure ASSETS ends up as a space-separated list
of all provided assets.
In `@Dockerfile`:
- Around line 11-13: The Dockerfile currently copies only go.mod and runs "go
mod download", which omits checksum verification; update the COPY instruction
that references "go.mod" (and the subsequent RUN "go mod download") to also copy
"go.sum" into the build context (e.g., change the COPY that targets go.mod to
include go.sum or use a wildcard like go.*) so that the build uses the lockfile
and preserves module checksum verification and reproducible builds.
- Around line 20-34: The container currently runs as root; create a non-root
user and run the image as that user by adding a USER creation step (e.g.,
addgroup/adduser or useradd) and switching to that user after preparing files
but before ENTRYPOINT; ensure /app/data and /app/output are owned by that user
(chown those paths) so the entrypoint script (docker-entrypoint.sh) and the
applications (indicator-sync, indicator-backtest) remain executable and writable
by the non-root user, and keep ENTRYPOINT ["docker-entrypoint.sh"] unchanged.
In `@README.md`:
- Around line 263-264: The README currently instructs macOS users to run "open
output/index.html" which is platform-specific; update that line to be
cross-platform by documenting both commands (e.g., "open output/index.html" for
macOS and "xdg-open output/index.html" for Linux) or provide a platform-agnostic
suggestion such as "use your browser to open output/index.html" so Linux users
are not blocked; make the change where the README contains the "open
output/index.html" instruction.
---
Nitpick comments:
In @.github/workflows/docker.yml:
- Around line 31-33: The workflow currently always pushes
ghcr.io/cinar/indicator:latest (via the tags block referencing github.ref_name),
which lets pre-release tags overwrite :latest; change the tagging step to gate
the latest tag to stable semver only — e.g. replace the static tags block with a
docker/metadata-action invocation (docker/metadata-action@v5) to generate tags
and only enable the raw latest tag when the tag name has no pre-release suffix
(use a condition like enable=${{ !contains(github.ref_name, '-') }}), then pass
the generated tags (steps.meta.outputs.tags) into docker/build-push-action so
pre-release refs (e.g. v2.0.0-beta) do not set :latest.
docker-entrypoint.sh
Outdated
| echo "==========================================" | ||
| echo "" | ||
| echo "Configuration:" | ||
| echo " API Key: ***${API_KEY:-none}" |
There was a problem hiding this comment.
API key is not masked — full key printed to stdout.
***${API_KEY:-none} expands to ***<the-actual-key>, leaking the secret to any log aggregator or terminal recording. The *** prefix is cosmetic only.
🔒 Proposed fix
-echo " API Key: ***${API_KEY:-none}"
+echo " API Key: ***"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo " API Key: ***${API_KEY:-none}" | |
| echo " API Key: ***" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker-entrypoint.sh` at line 80, The current echo prints the full API_KEY
(echo " API Key: ***${API_KEY:-none}") leaking secrets; change the echo to
avoid printing the raw value by replacing it with a masked or redacted
representation: compute a masked string from API_KEY (e.g., show "<redacted>" or
build a mask that keeps only the last 4 characters and replaces the rest with
asterisks) and use that masked variable in the echo, referencing the API_KEY
variable and the echo statement so the full secret is never emitted to stdout.
| COPY go.mod ./ | ||
|
|
||
| RUN go mod download |
There was a problem hiding this comment.
go.sum must be copied alongside go.mod.
Without go.sum, go mod download skips checksum verification and Go regenerates the lock file during the build, breaking reproducibility and bypassing module integrity guarantees.
🐛 Proposed fix
-COPY go.mod ./
+COPY go.mod go.sum ./📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| COPY go.mod ./ | |
| RUN go mod download | |
| COPY go.mod go.sum ./ | |
| RUN go mod download |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 11 - 13, The Dockerfile currently copies only go.mod
and runs "go mod download", which omits checksum verification; update the COPY
instruction that references "go.mod" (and the subsequent RUN "go mod download")
to also copy "go.sum" into the build context (e.g., change the COPY that targets
go.mod to include go.sum or use a wildcard like go.*) so that the build uses the
lockfile and preserves module checksum verification and reproducible builds.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docker-entrypoint.sh (1)
96-132: Unquoted$ASSETSis subject to glob expansion.The word-splitting behavior is intentional (POSIX
shhas no arrays), but unquoted$ASSETSalso undergoes pathname expansion. A ticker likecortthat matches a filename in the working directory would silently expand to filesystem paths instead of being passed as the ticker symbol. To split on spaces but not perform glob expansion, POSIX hasset -fto disable globbing.♻️ Proposed fix — guard both invocations with `set -f` / `set +f`
else + set -f + # shellcheck disable=SC2086 ./indicator-sync \ -source-name tiingo \ -source-config "$API_KEY" \ -target-name filesystem \ -target-config "$DATA_DIR" \ -days "$DAYS" \ $ASSETS + set +f fiApply the same wrapping to the
indicator-backtestinvocation on lines 125-132.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-entrypoint.sh` around lines 96 - 132, The unquoted $ASSETS can undergo pathname (glob) expansion; wrap the conditional invocations that pass $ASSETS to ./indicator-sync and ./indicator-backtest with a temporary globbing disable: before calling either command when $ASSETS is used, run set -f to disable pathname expansion, invoke ./indicator-sync or ./indicator-backtest with the unquoted $ASSETS, then run set +f to restore globbing; target the two call sites that reference $ASSETS (the ./indicator-sync and ./indicator-backtest branches) so word-splitting remains but glob expansion is prevented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-entrypoint.sh`:
- Around line 40-47: The flags parsing for --days and --last in
docker-entrypoint.sh currently assigns raw $2 to DAYS and LAST without
validation; add POSIX-compatible checks in the argument-handling branch for
these flags to ensure $2 exists and matches a positive-integer regex (e.g.
'^[0-9]+$'), otherwise print a clear user-friendly error mentioning the flag
(e.g. "--days requires a positive integer") to stderr and exit with a non-zero
status; update the --days and --last branches to validate $2 before assigning to
DAYS/LAST and shift only after successful validation.
- Around line 48-55: The --assets handling loop currently leaves ASSETS empty
when no tickers follow, causing later code to treat it as “all assets”; after
the while loop that builds ASSETS (the block using ASSETS="" and while [ $# -gt
0 ] && [ "${1#-}" = "$1" ]; do ... done), add an explicit check if [ -z
"$ASSETS" ] then print an error to stderr (e.g. echo "Error: --assets requires
at least one ticker" >&2) and exit 1 so the script fails fast when --assets is
provided with no arguments.
In `@README.md`:
- Around line 298-304: The example under "Custom output directory" redundantly
passes the default --output value; either remove the --output /app/output
argument or change it to a non-default container path to demonstrate real
customization. Update the README example by removing the --output flag so the
host mount (-v /path/to/my/reports:/app/output) is the only change, or
alternatively set the container path in the flag to something different (e.g.,
--output /custom/output) and adjust the volume mount accordingly; reference the
example's docker run command and the --output flag when making the edit.
---
Nitpick comments:
In `@docker-entrypoint.sh`:
- Around line 96-132: The unquoted $ASSETS can undergo pathname (glob)
expansion; wrap the conditional invocations that pass $ASSETS to
./indicator-sync and ./indicator-backtest with a temporary globbing disable:
before calling either command when $ASSETS is used, run set -f to disable
pathname expansion, invoke ./indicator-sync or ./indicator-backtest with the
unquoted $ASSETS, then run set +f to restore globbing; target the two call sites
that reference $ASSETS (the ./indicator-sync and ./indicator-backtest branches)
so word-splitting remains but glob expansion is prevented.
| --days) | ||
| DAYS="$2" | ||
| shift 2 | ||
| ;; | ||
| --last) | ||
| LAST="$2" | ||
| shift 2 | ||
| ;; |
There was a problem hiding this comment.
--days and --last accept non-numeric values without user-friendly error.
There is no validation that $2 is a positive integer for either flag. A typo (e.g., --days foo) or omitting the value (e.g., --days as the final argument) silently passes an empty or non-numeric string to the Go binary, producing a cryptic flag-parse error rather than a friendly message.
🛡️ Proposed fix — add POSIX-compatible numeric validation
--days)
+ case "$2" in
+ ''|*[!0-9]*) echo "Error: --days must be a positive integer"; exit 1 ;;
+ esac
DAYS="$2"
shift 2
;;
--last)
+ case "$2" in
+ ''|*[!0-9]*) echo "Error: --last must be a positive integer"; exit 1 ;;
+ esac
LAST="$2"
shift 2
;;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| --days) | |
| DAYS="$2" | |
| shift 2 | |
| ;; | |
| --last) | |
| LAST="$2" | |
| shift 2 | |
| ;; | |
| --days) | |
| case "$2" in | |
| ''|*[!0-9]*) echo "Error: --days must be a positive integer"; exit 1 ;; | |
| esac | |
| DAYS="$2" | |
| shift 2 | |
| ;; | |
| --last) | |
| case "$2" in | |
| ''|*[!0-9]*) echo "Error: --last must be a positive integer"; exit 1 ;; | |
| esac | |
| LAST="$2" | |
| shift 2 | |
| ;; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker-entrypoint.sh` around lines 40 - 47, The flags parsing for --days and
--last in docker-entrypoint.sh currently assigns raw $2 to DAYS and LAST without
validation; add POSIX-compatible checks in the argument-handling branch for
these flags to ensure $2 exists and matches a positive-integer regex (e.g.
'^[0-9]+$'), otherwise print a clear user-friendly error mentioning the flag
(e.g. "--days requires a positive integer") to stderr and exit with a non-zero
status; update the --days and --last branches to validate $2 before assigning to
DAYS/LAST and shift only after successful validation.
| --assets) | ||
| shift | ||
| ASSETS="" | ||
| while [ $# -gt 0 ] && [ "${1#-}" = "$1" ]; do | ||
| ASSETS="${ASSETS:+$ASSETS }$1" | ||
| shift | ||
| done | ||
| ;; |
There was a problem hiding this comment.
--assets with no following tickers silently falls back to "all assets."
When --assets is the last argument (or is immediately followed by another flag), the inner loop exits with ASSETS="". The later if [ -z "$ASSETS" ] check then silently runs for all assets, contrary to what the user likely intended. An explicit error here prevents silent misuse.
🛡️ Proposed fix — error when no tickers follow `--assets`
--assets)
shift
ASSETS=""
while [ $# -gt 0 ] && [ "${1#-}" = "$1" ]; do
ASSETS="${ASSETS:+$ASSETS }$1"
shift
done
+ if [ -z "$ASSETS" ]; then
+ echo "Error: --assets requires at least one ticker symbol"
+ show_usage
+ exit 1
+ fi
;;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| --assets) | |
| shift | |
| ASSETS="" | |
| while [ $# -gt 0 ] && [ "${1#-}" = "$1" ]; do | |
| ASSETS="${ASSETS:+$ASSETS }$1" | |
| shift | |
| done | |
| ;; | |
| --assets) | |
| shift | |
| ASSETS="" | |
| while [ $# -gt 0 ] && [ "${1#-}" = "$1" ]; do | |
| ASSETS="${ASSETS:+$ASSETS }$1" | |
| shift | |
| done | |
| if [ -z "$ASSETS" ]; then | |
| echo "Error: --assets requires at least one ticker symbol" | |
| show_usage | |
| exit 1 | |
| fi | |
| ;; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker-entrypoint.sh` around lines 48 - 55, The --assets handling loop
currently leaves ASSETS empty when no tickers follow, causing later code to
treat it as “all assets”; after the while loop that builds ASSETS (the block
using ASSETS="" and while [ $# -gt 0 ] && [ "${1#-}" = "$1" ]; do ... done), add
an explicit check if [ -z "$ASSETS" ] then print an error to stderr (e.g. echo
"Error: --assets requires at least one ticker" >&2) and exit 1 so the script
fails fast when --assets is provided with no arguments.
| # Custom output directory | ||
| docker run -it --rm \ | ||
| -v /path/to/my/reports:/app/output \ | ||
| ghcr.io/cinar/indicator:latest \ | ||
| --api-key YOUR_TIINGO_API_KEY \ | ||
| --output /app/output | ||
| ``` |
There was a problem hiding this comment.
"Custom output directory" example redundantly passes the default --output value.
The --output /app/output argument on Line 303 restates the default, adding no new information. The example's actual customization is the host-side volume mount (-v /path/to/my/reports:/app/output), which works identically without the --output flag. Either drop the flag or show a non-default container path to make the example meaningful.
📝 Option A — remove the redundant flag
# Custom output directory
docker run -it --rm \
-v /path/to/my/reports:/app/output \
ghcr.io/cinar/indicator:latest \
- --api-key YOUR_TIINGO_API_KEY \
- --output /app/output
+ --api-key YOUR_TIINGO_API_KEY📝 Option B — demonstrate a genuinely custom container-side path
# Custom output directory
docker run -it --rm \
- -v /path/to/my/reports:/app/output \
+ -v /path/to/my/reports:/app/reports \
ghcr.io/cinar/indicator:latest \
--api-key YOUR_TIINGO_API_KEY \
- --output /app/output
+ --output /app/reports📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Custom output directory | |
| docker run -it --rm \ | |
| -v /path/to/my/reports:/app/output \ | |
| ghcr.io/cinar/indicator:latest \ | |
| --api-key YOUR_TIINGO_API_KEY \ | |
| --output /app/output | |
| ``` | |
| # Custom output directory | |
| docker run -it --rm \ | |
| -v /path/to/my/reports:/app/output \ | |
| ghcr.io/cinar/indicator:latest \ | |
| --api-key YOUR_TIINGO_API_KEY |
| # Custom output directory | |
| docker run -it --rm \ | |
| -v /path/to/my/reports:/app/output \ | |
| ghcr.io/cinar/indicator:latest \ | |
| --api-key YOUR_TIINGO_API_KEY \ | |
| --output /app/output | |
| ``` | |
| # Custom output directory | |
| docker run -it --rm \ | |
| -v /path/to/my/reports:/app/reports \ | |
| ghcr.io/cinar/indicator:latest \ | |
| --api-key YOUR_TIINGO_API_KEY \ | |
| --output /app/reports |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 298 - 304, The example under "Custom output
directory" redundantly passes the default --output value; either remove the
--output /app/output argument or change it to a non-default container path to
demonstrate real customization. Update the README example by removing the
--output flag so the host mount (-v /path/to/my/reports:/app/output) is the only
change, or alternatively set the container path in the flag to something
different (e.g., --output /custom/output) and adjust the volume mount
accordingly; reference the example's docker run command and the --output flag
when making the edit.
2 participants
Describe Request
Docker image support added.
Fixed #288
Change Type
New feature.
Summary by CodeRabbit
New Features
Documentation