Add make targets for local development setup and documentation - #137
Add make targets for local development setup and documentation#137PuneetPunamiya wants to merge 2 commits into
Conversation
GitHub Actions
Failure
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces automated local development environment setup and cleanup through new Makefile targets and bash scripts. The scripts handle Kind cluster creation, namespace setup, local caching, Docling service management, and LocalStack resource configuration. Feedback was provided to improve the scripts' robustness by adding dependency checks, using relative paths for configuration files, removing unused variables, and ensuring the cleanup process removes the local cache directory.
3f32894 to
f6639f8
Compare
GitHub Actions
Failure
PuneetPunamiya
changed the title
f6639f8 to
f28a507
Compare
GitHub Actions
Failure
f28a507 to
37217a9
Compare
GitHub Actions
Failure
37217a9 to
f940fad
Compare
GitHub Actions
Failure
|
Can we priortise this? I would like to try this as customer 0. Thanks. |
| #!/usr/bin/env bash | ||
| set -e | ||
|
|
||
| export AWS_DEFAULT_REGION="${AWS_DEFAULT_REGION:-us-east-1}" |
There was a problem hiding this comment.
can we move this to yaml
| CACHE_DIR="${CACHE_DIR:-tmp/cache}" | ||
| DATA_STORAGE_BUCKET="${DATA_STORAGE_BUCKET:-data-storage-bucket}" | ||
| DATA_INGESTION_BUCKET="${DATA_INGESTION_BUCKET:-data-ingestion-bucket}" | ||
| OUTPUT_RESULT_BUCKET="${OUTPUT_RESULT_BUCKET:-output-result-bucket}" |
There was a problem hiding this comment.
why such name?
can we follow the convention defined
| CACHE_DIR="${CACHE_DIR:-tmp/cache}" | ||
| DATA_STORAGE_BUCKET="${DATA_STORAGE_BUCKET:-data-storage-bucket}" | ||
| DATA_INGESTION_BUCKET="${DATA_INGESTION_BUCKET:-data-ingestion-bucket}" | ||
| OUTPUT_RESULT_BUCKET="${OUTPUT_RESULT_BUCKET:-output-result-bucket}" |
There was a problem hiding this comment.
why we are introducing these variables in script, these should be part of yaml
this is defining them again, we can keep kind and namespace thing
| echo "✓ Docling is already running" | ||
| else | ||
| echo "Starting Docling in the background..." | ||
| nohup docling-serve run --enable-ui > /tmp/docling.log 2>&1 & |
There was a problem hiding this comment.
this will create dep to install this and not sure how it will work on other system, can we do via sh
| --bucket "${DATA_INGESTION_BUCKET}" \ | ||
| --notification-configuration "file://${NOTIFICATION_FILE}" | ||
| echo "✓ S3 bucket notification configured (queue ARN from LocalStack)" | ||
|
|
There was a problem hiding this comment.
what about ollama setup
| echo "✓ S3 bucket notification configured (queue ARN from LocalStack)" | ||
|
|
||
| echo "" | ||
| echo "✓ Local development environment setup complete!" |
There was a problem hiding this comment.
dont we need to create create and controller config also, or we can do till make install and run
| ```bash | ||
| make run | ||
| ``` | ||
|
|
There was a problem hiding this comment.
make local dev should cover till here i think, i will let @concaf chime in
|
|
||
| ```yaml | ||
| apiVersion: operator.dataverse.redhat.com/v1alpha1 | ||
| kind: SQSInformer |
There was a problem hiding this comment.
this can be created also, as queue is getting created in script
| ### Export Environment Variables | ||
|
|
||
| ```bash | ||
| export IMG=<your-image-registry>/<image-name>:<tag> |
There was a problem hiding this comment.
we can take these variables as input and create controller config and secret also
f940fad to
5541946
Compare
GitHub Actions
Failure
Add DEVELOPMENT.md guide covering the complete local development workflow including LocalStack setup, CRD installation, controller configuration, and e2e testing with required environment variables Introduce make targets (local-dev-setup, local-dev-cleanup) with corresponding shell scripts to automate local environment creation, including S3 buckets, SQS queues, and Docling service setup. Fix test-e2e target to pass KIND_CLUSTER and SKIP_CLUSTER_SETUP environment variables, preventing duplicate cluster creation and aligning local test execution with CI workflow. Signed-off-by: Puneet Punamiya <ppunamiy@redhat.com>
5541946 to
04b0bf5
Compare
📝 WalkthroughWalkthroughThis PR adds tag-triggered GitHub draft release automation, a release Bash script for minor and patch versions, local development setup and cleanup scripts, LocalStack lifecycle commands, Make targets, e2e cleanup handling, and development documentation. ChangesRelease automation
Local development environment
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant release.sh
participant Git
participant GitHubCLI
GitHubActions->>release.sh: invoke minor or patch release
release.sh->>Git: inspect tags and commits
release.sh->>GitHubCLI: create draft GitHub release
GitHubCLI-->>GitHubActions: provide release URL and image tags
sequenceDiagram
participant Developer
participant local-dev-setup.sh
participant Kind
participant LocalStack
participant Kubernetes
Developer->>local-dev-setup.sh: run local development setup
local-dev-setup.sh->>Kind: create or reuse cluster
local-dev-setup.sh->>LocalStack: start and configure buckets
local-dev-setup.sh->>Kubernetes: install CRDs and apply resources
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 10
🧹 Nitpick comments (3)
.github/workflows/release.yml (2)
16-20: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider a
concurrencygroup.Two tags pushed close together both run this job; the minor path creates and pushes
release-v0.X.x, so concurrent runs can race on branch creation and push. Aconcurrency: group: release-${{ github.ref }}withcancel-in-progress: falseserializes them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 16 - 20, Add a concurrency configuration to the release workflow’s release job, using a group keyed by github.ref and setting cancel-in-progress to false. This must serialize runs for the same reference while allowing queued releases to complete without cancellation.
29-32: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuePin
actions/checkoutto a commit SHA. This workflow has write access and keeps credentials for the release-push path, so a mutable tag leaves avoidable supply-chain risk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 29 - 32, Update the “Checkout repository” step in the release workflow to pin actions/checkout to a specific immutable commit SHA instead of the mutable v6 tag. Keep fetch-depth set to 0 and preserve the existing release workflow behavior.Source: Linters/SAST tools
hack/release.sh (1)
94-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
grep -Pis GNU-only — fails on macOS.BSD
grep(default on macOS) rejects-P, so these helpers error out for maintainers running the script locally, which is the primary documented use case. Portable alternatives keep the same behavior.♻️ Portable variants
get_latest_minor() { - get_all_tags | { grep -oP 'v\K[0-9]+\.[0-9]+' || true; } | sort -t. -k1,1n -k2,2n | tail -1 + get_all_tags | sed -nE 's/^v([0-9]+\.[0-9]+).*/\1/p' | sort -t. -k1,1n -k2,2n | tail -1 } get_latest_patch_for_minor() { local minor="$1" - get_all_tags | { grep -P "^v${minor}\.[0-9]+$" || true; } | sort -V | tail -1 + get_all_tags | { grep -E "^v${minor}\.[0-9]+$" || true; } | sort -V | tail -1 } get_max_patch_number() { local minor="$1" - get_all_tags | { grep -P "^v${minor}\.[0-9]+$" || true; } | { grep -oP '\.[0-9]+$' || true; } | tr -d '.' | sort -n | tail -1 + get_all_tags | { grep -E "^v${minor}\.[0-9]+$" || true; } | sed -nE 's/.*\.([0-9]+)$/\1/p' | sort -n | tail -1 }Line 285 uses
grep -Ptoo.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/release.sh` around lines 94 - 106, Replace every GNU-specific grep -P/-oP usage in get_latest_minor, get_latest_patch_for_minor, get_max_patch_number, and the additional call around line 285 with portable shell filtering that preserves the existing tag matching and extraction behavior on macOS BSD grep. Ensure no grep -P flags remain in the script.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Around line 21-27: Prevent shell injection from github.ref_name by passing it
through step-level env variables instead of interpolating it in run scripts. In
.github/workflows/release.yml#L21-L27, add TAG from github.ref_name and use
"$TAG"; apply the same TAG mapping and remove inline expansion at `#L34-L37`,
`#L47-L53`, and `#L55-L61`, adding MINOR_VERSION where specified and passing shell
variables to hack/release.sh. At `#L63-L77`, add TAG, RELEASE_TYPE, and
MINOR_VERSION to env and use those shell variables in the summary block.
In `@hack/release.sh`:
- Around line 27-34: Replace eval-based execution in run_cmd with direct
argument execution, preserving each argument’s quoting and preventing shell
metacharacters from being re-parsed. Update the notes_flag construction and its
call site to use an array so optional notes arguments expand safely without
relying on unquoted splitting.
- Around line 127-144: Validate VERSION_OVERRIDE before deriving next_minor_num
in the override branch of the release script, accepting only the version format
supported by this v0.x release flow. For invalid values such as v1.0.0 or
malformed strings, emit a clear error and terminate explicitly; only then
compute release_branch, prev_release_branch, and prev_tag.
- Around line 274-295: Update the VERSION_OVERRIDE branch that computes
latest_patch_tag to resolve the highest existing v${minor}.x tag below
next_version rather than assuming override_patch - 1 exists. Use the target
tag’s parent history or an equivalent existing-tag lookup, ensuring an
already-pushed target resolves to its exact predecessor and missing/skipped tags
do not cause git rev-list to fail.
- Around line 391-394: Update the unknown-argument branch in the release script
and the [[ -z "$COMMAND" ]] fallthrough to terminate with a nonzero status after
displaying usage, rather than calling usage() with its successful exit. Preserve
normal usage behavior for explicitly requested help and successful command
execution.
In `@Makefile`:
- Around line 145-146: Update the e2e test target around the go test invocation
and cleanup-test-e2e to execute both commands in one shell, capture the go test
exit status, always run cleanup even when tests fail, and return the original
test status after cleanup.
In `@scripts/local-dev-cleanup.sh`:
- Around line 38-45: Harden CACHE_DIR construction and the cleanup guard in
scripts/local-dev-cleanup.sh: ensure failed or empty yq output cannot produce
the repository root, normalize the resolved path, and validate it is non-empty,
not equal to REPO_ROOT, and remains within the repository before rm -rf. Keep
the existing cache removal and success message for validated directories.
In `@scripts/local-dev-setup.sh`:
- Around line 58-59: Replace predictable /tmp process-state paths with a
per-user mode-0700 protected runtime directory, preferably under
XDG_RUNTIME_DIR, and ensure setup creates or validates it. In
scripts/local-dev-setup.sh lines 58-59, store Docling PID/log files there; in
scripts/local-dev-setup.sh lines 79-80, store Ollama PID/log files there; and in
scripts/local-dev-cleanup.sh lines 11-25, read and remove only those
corresponding protected files.
- Around line 88-91: Update the embedding model provisioning block in the local
setup script so a failed ollama pull does not continue to the “Embedding model
ready” message; propagate the pull failure and stop setup. Treat the ollama cp
alias operation separately, allowing only that optional alias failure while
still reporting readiness solely after a successful nomic-embed-text:latest
pull.
In `@scripts/localstack.sh`:
- Around line 20-22: Move the INGESTION_BUCKET, DATA_STORAGE_BUCKET, and
OUTPUT_BUCKET resolutions out of the top-level script flow and into cmd_setup,
after require_command yq runs. Keep these values available to the setup logic,
while allowing start, stop, and status to dispatch without evaluating yq or
validating the YAML files.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 16-20: Add a concurrency configuration to the release workflow’s
release job, using a group keyed by github.ref and setting cancel-in-progress to
false. This must serialize runs for the same reference while allowing queued
releases to complete without cancellation.
- Around line 29-32: Update the “Checkout repository” step in the release
workflow to pin actions/checkout to a specific immutable commit SHA instead of
the mutable v6 tag. Keep fetch-depth set to 0 and preserve the existing release
workflow behavior.
In `@hack/release.sh`:
- Around line 94-106: Replace every GNU-specific grep -P/-oP usage in
get_latest_minor, get_latest_patch_for_minor, get_max_patch_number, and the
additional call around line 285 with portable shell filtering that preserves the
existing tag matching and extraction behavior on macOS BSD grep. Ensure no grep
-P flags remain in the script.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Enterprise
Run ID: 7776f8ec-d1d3-4982-ac23-fcd270dda37c
📒 Files selected for processing (7)
.github/workflows/release.ymlDEVELOPMENT.mdMakefilehack/release.shscripts/local-dev-cleanup.shscripts/local-dev-setup.shscripts/localstack.sh
| - name: Validate tag format | ||
| run: | | ||
| TAG="${{ github.ref_name }}" | ||
| if [[ ! "$TAG" =~ ^v[0-9]+\.[0-9]+\.[0-9]+$ ]]; then | ||
| echo "::error::Invalid tag format: ${TAG}. Expected vMAJOR.MINOR.PATCH (e.g., v0.12.0)" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Untrusted github.ref_name is expanded into every run: body. GitHub substitutes ${{ ... }} into the script text before the shell runs, and git ref names may contain $, backticks, ;, |, and quotes — so anyone able to push a tag gets arbitrary command execution in a job holding contents: write and persisted push credentials. The tag-format check cannot help, because it is itself written with an interpolated value. Fix by passing the value through env: and quoting the variable in every step.
.github/workflows/release.yml#L21-L27: addenv: TAG: ${{ github.ref_name }}to the validation step and useif [[ ! "$TAG" =~ ... ]]..github/workflows/release.yml#L34-L37: add the sameenv: TAG:mapping and drop the inline expansion at Line 37..github/workflows/release.yml#L47-L53: addTAG:to the existingenv:block and callbash hack/release.sh minor --version "$TAG" --force..github/workflows/release.yml#L55-L61: addTAG:andMINOR_VERSION: ${{ steps.info.outputs.minor_version }}toenv:and callbash hack/release.sh patch "$MINOR_VERSION" --version "$TAG" --force..github/workflows/release.yml#L63-L77: addTAG:,RELEASE_TYPE:, andMINOR_VERSION:toenv:and reference the shell variables in the summary block.
🧰 Tools
🪛 zizmor (1.28.0)
[error] 23-23: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
📍 Affects 1 file
.github/workflows/release.yml#L21-L27(this comment).github/workflows/release.yml#L34-L37.github/workflows/release.yml#L47-L53.github/workflows/release.yml#L55-L61.github/workflows/release.yml#L63-L77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 21 - 27, Prevent shell injection
from github.ref_name by passing it through step-level env variables instead of
interpolating it in run scripts. In .github/workflows/release.yml#L21-L27, add
TAG from github.ref_name and use "$TAG"; apply the same TAG mapping and remove
inline expansion at `#L34-L37`, `#L47-L53`, and `#L55-L61`, adding MINOR_VERSION where
specified and passing shell variables to hack/release.sh. At `#L63-L77`, add TAG,
RELEASE_TYPE, and MINOR_VERSION to env and use those shell variables in the
summary block.
Source: Linters/SAST tools
| run_cmd() { | ||
| if $DRY_RUN; then | ||
| dry_run "$*" | ||
| else | ||
| info "Running: $*" | ||
| eval "$@" | ||
| fi | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Drop eval in run_cmd; execute arguments directly.
eval "$@" re-parses every argument as shell code, so any version/branch/repo string containing shell metacharacters (all derived from git refs or --version) is executed. It also destroys quoting. The only reason eval is needed today is the deliberately unquoted $notes_flag at Line 227 — use an array there instead.
🔒️ Proposed fix: array-based execution
run_cmd() {
if $DRY_RUN; then
dry_run "$*"
else
info "Running: $*"
- eval "$@"
+ "$@"
fi
}And at Lines 219-227:
- local notes_flag=""
+ local notes_flag=()
if [[ -n "$prev_tag" ]]; then
- notes_flag="--notes-start-tag ${prev_tag}"
+ notes_flag=(--notes-start-tag "$prev_tag")
fi
run_cmd gh release create "$next_version" \
--repo "$REPO" \
--generate-notes \
--draft \
- $notes_flag
+ "${notes_flag[@]}"🧰 Tools
🪛 ast-grep (0.45.0)
[error] 31-31: eval is invoked on a variable, parameter expansion, or command-substitution result, which re-parses the value as shell code. If any part of that value is attacker-controlled (arguments, environment, file contents, network output), it allows arbitrary command execution. Do not eval dynamic data: invoke the command directly with proper quoting (e.g. "$cmd" "$arg"), use arrays for argument lists (cmd=(prog --flag "$value"); "${cmd[@]}"), or restrict input to a validated allowlist before running it.
Context: eval "$@"
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(eval-on-variable-bash)
🪛 Shellcheck (0.11.0)
[warning] 32-32: eval negates the benefit of arrays. Drop eval to preserve whitespace/symbols (or eval as string).
(SC2294)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hack/release.sh` around lines 27 - 34, Replace eval-based execution in
run_cmd with direct argument execution, preserving each argument’s quoting and
preventing shell metacharacters from being re-parsed. Update the notes_flag
construction and its call site to use an array so optional notes arguments
expand safely without relying on unquoted splitting.
Source: Linters/SAST tools
| if [[ -n "$VERSION_OVERRIDE" ]]; then | ||
| next_version="${VERSION_OVERRIDE}" | ||
| next_minor_num=$(echo "$next_version" | grep -oP 'v0\.\K[0-9]+') | ||
| release_branch="release-v0.${next_minor_num}.x" | ||
|
|
||
| local prev_minor_num=$((next_minor_num - 1)) | ||
| prev_release_branch="release-v0.${prev_minor_num}.x" | ||
| prev_tag=$(get_latest_patch_for_minor "0.${prev_minor_num}") | ||
| else | ||
| local latest_minor_num | ||
| latest_minor_num=$(echo "$latest_minor" | cut -d. -f2) | ||
| next_minor_num=$((latest_minor_num + 1)) | ||
| next_version="v0.${next_minor_num}.0" | ||
| release_branch="release-v0.${next_minor_num}.x" | ||
|
|
||
| prev_release_branch="release-v0.${latest_minor_num}.x" | ||
| prev_tag=$(get_latest_patch_for_minor "0.${latest_minor_num}") | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--version isn't validated, and non-v0.x values fail silently.
grep -oP 'v0\.\K[0-9]+' only matches v0.*. For v1.0.0 (which the workflow's tag validation accepts) the substitution produces no output and the script either aborts with no message under set -e or continues with release-v0..x / release-v0.-1.x. Validate the override up front.
🐛 Proposed fix
if [[ -n "$VERSION_OVERRIDE" ]]; then
+ if [[ ! "$VERSION_OVERRIDE" =~ ^v0\.([0-9]+)\.0$ ]]; then
+ error "Invalid --version for a minor release: ${VERSION_OVERRIDE} (expected v0.X.0)"
+ exit 1
+ fi
next_version="${VERSION_OVERRIDE}"
- next_minor_num=$(echo "$next_version" | grep -oP 'v0\.\K[0-9]+')
+ next_minor_num="${BASH_REMATCH[1]}"
release_branch="release-v0.${next_minor_num}.x"📝 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.
| if [[ -n "$VERSION_OVERRIDE" ]]; then | |
| next_version="${VERSION_OVERRIDE}" | |
| next_minor_num=$(echo "$next_version" | grep -oP 'v0\.\K[0-9]+') | |
| release_branch="release-v0.${next_minor_num}.x" | |
| local prev_minor_num=$((next_minor_num - 1)) | |
| prev_release_branch="release-v0.${prev_minor_num}.x" | |
| prev_tag=$(get_latest_patch_for_minor "0.${prev_minor_num}") | |
| else | |
| local latest_minor_num | |
| latest_minor_num=$(echo "$latest_minor" | cut -d. -f2) | |
| next_minor_num=$((latest_minor_num + 1)) | |
| next_version="v0.${next_minor_num}.0" | |
| release_branch="release-v0.${next_minor_num}.x" | |
| prev_release_branch="release-v0.${latest_minor_num}.x" | |
| prev_tag=$(get_latest_patch_for_minor "0.${latest_minor_num}") | |
| fi | |
| if [[ -n "$VERSION_OVERRIDE" ]]; then | |
| if [[ ! "$VERSION_OVERRIDE" =~ ^v0\.([0-9]+)\.0$ ]]; then | |
| error "Invalid --version for a minor release: ${VERSION_OVERRIDE} (expected v0.X.0)" | |
| exit 1 | |
| fi | |
| next_version="${VERSION_OVERRIDE}" | |
| next_minor_num="${BASH_REMATCH[1]}" | |
| release_branch="release-v0.${next_minor_num}.x" | |
| local prev_minor_num=$((next_minor_num - 1)) | |
| prev_release_branch="release-v0.${prev_minor_num}.x" | |
| prev_tag=$(get_latest_patch_for_minor "0.${prev_minor_num}") | |
| else | |
| local latest_minor_num | |
| latest_minor_num=$(echo "$latest_minor" | cut -d. -f2) | |
| next_minor_num=$((latest_minor_num + 1)) | |
| next_version="v0.${next_minor_num}.0" | |
| release_branch="release-v0.${next_minor_num}.x" | |
| prev_release_branch="release-v0.${latest_minor_num}.x" | |
| prev_tag=$(get_latest_patch_for_minor "0.${latest_minor_num}") | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hack/release.sh` around lines 127 - 144, Validate VERSION_OVERRIDE before
deriving next_minor_num in the override branch of the release script, accepting
only the version format supported by this v0.x release flow. For invalid values
such as v1.0.0 or malformed strings, emit a clear error and terminate
explicitly; only then compute release_branch, prev_release_branch, and prev_tag.
| if [[ -n "$VERSION_OVERRIDE" ]]; then | ||
| next_version="${VERSION_OVERRIDE}" | ||
| local override_patch="${next_version##*.}" | ||
| if (( override_patch > 0 )); then | ||
| latest_patch_tag="v${minor}.$((override_patch - 1))" | ||
| fi | ||
| else | ||
| local next_patch=$((max_patch + 1)) | ||
| next_version="v${minor}.${next_patch}" | ||
| fi | ||
|
|
||
| info "Existing v${minor}.x tags: $(get_all_tags | grep -P "^v${minor}\.[0-9]+$" | tr '\n' ' ')" | ||
| info "Latest patch: ${latest_patch_tag}" | ||
| info "Next version: ${BOLD}${next_version}${NC}" | ||
|
|
||
| if ! git rev-parse "$next_version" &>/dev/null 2>&1; then | ||
| error "Tag ${next_version} does not exist. Create and push the tag first." | ||
| exit 1 | ||
| fi | ||
|
|
||
| local new_commits | ||
| new_commits=$(git rev-list --count "${latest_patch_tag}..${UPSTREAM_REMOTE}/${release_branch}") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Derived predecessor tag may not exist, aborting the release.
With --version, latest_patch_tag is computed as v${minor}.$((override_patch - 1)) without checking that the tag exists. If a patch number was skipped or a tag deleted (e.g. v0.10.1 → v0.10.3), the assumed v0.10.2 is missing and git rev-list at Line 295 fails under set -e, aborting the whole release. Fall back to the highest real tag below the target.
🐛 Proposed fix
local override_patch="${next_version##*.}"
- if (( override_patch > 0 )); then
- latest_patch_tag="v${minor}.$((override_patch - 1))"
+ if [[ "$override_patch" =~ ^[0-9]+$ ]] && (( override_patch > 0 )); then
+ local candidate="v${minor}.$((override_patch - 1))"
+ if git rev-parse -q --verify "refs/tags/${candidate}" >/dev/null; then
+ latest_patch_tag="$candidate"
+ fi
fiNote latest_patch_tag then remains the highest existing v${minor}.x tag, which is $next_version itself when the tag is already pushed — consider git describe --tags --abbrev=0 "${next_version}^" for an exact predecessor.
📝 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.
| if [[ -n "$VERSION_OVERRIDE" ]]; then | |
| next_version="${VERSION_OVERRIDE}" | |
| local override_patch="${next_version##*.}" | |
| if (( override_patch > 0 )); then | |
| latest_patch_tag="v${minor}.$((override_patch - 1))" | |
| fi | |
| else | |
| local next_patch=$((max_patch + 1)) | |
| next_version="v${minor}.${next_patch}" | |
| fi | |
| info "Existing v${minor}.x tags: $(get_all_tags | grep -P "^v${minor}\.[0-9]+$" | tr '\n' ' ')" | |
| info "Latest patch: ${latest_patch_tag}" | |
| info "Next version: ${BOLD}${next_version}${NC}" | |
| if ! git rev-parse "$next_version" &>/dev/null 2>&1; then | |
| error "Tag ${next_version} does not exist. Create and push the tag first." | |
| exit 1 | |
| fi | |
| local new_commits | |
| new_commits=$(git rev-list --count "${latest_patch_tag}..${UPSTREAM_REMOTE}/${release_branch}") | |
| if [[ -n "$VERSION_OVERRIDE" ]]; then | |
| next_version="${VERSION_OVERRIDE}" | |
| local override_patch="${next_version##*.}" | |
| if [[ "$override_patch" =~ ^[0-9]+$ ]] && (( override_patch > 0 )); then | |
| local candidate="v${minor}.$((override_patch - 1))" | |
| if git rev-parse -q --verify "refs/tags/${candidate}" >/dev/null; then | |
| latest_patch_tag="$candidate" | |
| fi | |
| fi | |
| else | |
| local next_patch=$((max_patch + 1)) | |
| next_version="v${minor}.${next_patch}" | |
| fi | |
| info "Existing v${minor}.x tags: $(get_all_tags | grep -P "^v${minor}\.[0-9]+$" | tr '\n' ' ')" | |
| info "Latest patch: ${latest_patch_tag}" | |
| info "Next version: ${BOLD}${next_version}${NC}" | |
| if ! git rev-parse "$next_version" &>/dev/null 2>&1; then | |
| error "Tag ${next_version} does not exist. Create and push the tag first." | |
| exit 1 | |
| fi | |
| local new_commits | |
| new_commits=$(git rev-list --count "${latest_patch_tag}..${UPSTREAM_REMOTE}/${release_branch}") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hack/release.sh` around lines 274 - 295, Update the VERSION_OVERRIDE branch
that computes latest_patch_tag to resolve the highest existing v${minor}.x tag
below next_version rather than assuming override_patch - 1 exists. Use the
target tag’s parent history or an equivalent existing-tag lookup, ensuring an
already-pushed target resolves to its exact predecessor and missing/skipped tags
do not cause git rev-list to fail.
| *) | ||
| error "Unknown argument: $1" | ||
| usage | ||
| ;; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Unknown arguments exit 0.
usage() ends with exit 0, so a typo'd flag prints an error and then exits successfully — CI would treat a misinvocation as a passing release step. Same applies to the [[ -z "$COMMAND" ]] fallthrough at Lines 398-400.
🐛 Proposed fix
usage() {
cat <<EOF
...
EOF
- exit 0
+ exit "${1:-0}"
} *)
error "Unknown argument: $1"
- usage
+ usage 1
;;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hack/release.sh` around lines 391 - 394, Update the unknown-argument branch
in the release script and the [[ -z "$COMMAND" ]] fallthrough to terminate with
a nonzero status after displaying usage, rather than calling usage() with its
successful exit. Preserve normal usage behavior for explicitly requested help
and successful command execution.
| KIND_CLUSTER=$(KIND_CLUSTER) SKIP_CLUSTER_SETUP=true go test -count=1 -tags e2e ./test/e2e/ -v -timeout=60m | ||
| $(MAKE) cleanup-test-e2e |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Always clean up the Kind cluster after a failed test run.
A non-zero go test causes Make to stop before line 146, leaving the test cluster behind. Run the test and cleanup in one shell with a captured exit status (or a trap), then return the original test failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` around lines 145 - 146, Update the e2e test target around the go
test invocation and cleanup-test-e2e to execute both commands in one shell,
capture the go test exit status, always run cleanup even when tests fail, and
return the original test status after cleanup.
| CACHE_DIR="${REPO_ROOT}/$( | ||
| yq -r '.spec.cacheDirectory' \ | ||
| "${REPO_ROOT}/config/samples/operator_v1alpha1_controllerconfig.yaml" 2>/dev/null \ | ||
| | sed 's|/$||' || echo "tmp/cache" | ||
| )" | ||
| if [ -d "${CACHE_DIR}" ]; then | ||
| rm -rf "${CACHE_DIR}" | ||
| echo "✓ Removed cache directory ${CACHE_DIR}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Prevent cleanup from deleting the repository root.
Without pipefail, a failed yq is masked by sed; the substitution becomes empty and CACHE_DIR resolves to ${REPO_ROOT}/. Line 44 then recursively removes the checkout. Require a non-empty cache path and reject paths that resolve to the repository root or escape it before calling rm -rf.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/local-dev-cleanup.sh` around lines 38 - 45, Harden CACHE_DIR
construction and the cleanup guard in scripts/local-dev-cleanup.sh: ensure
failed or empty yq output cannot produce the repository root, normalize the
resolved path, and validate it is non-empty, not equal to REPO_ROOT, and remains
within the repository before rm -rf. Keep the existing cache removal and success
message for validated directories.
| nohup docling-serve run --enable-ui > /tmp/docling.log 2>&1 & | ||
| echo $! > /tmp/docling.pid |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Store managed-process state in a private runtime directory.
Predictable /tmp PID/log paths let another local user pre-create or replace state files. In particular, cleanup reads the PID and may signal an attacker-selected process. Use a per-user, mode-0700 state directory (for example below ${XDG_RUNTIME_DIR}), and have setup and cleanup use the same protected paths.
scripts/local-dev-setup.sh#L58-L59: write Docling PID and logs beneath the protected state directory.scripts/local-dev-setup.sh#L79-L80: write Ollama PID and logs beneath the protected state directory.scripts/local-dev-cleanup.sh#L11-L25: read and remove only the corresponding protected state files.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 58-58: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: /tmp/docling.pid
Note: [CWE-377] Insecure Temporary File.
(predictable-tmp-file-bash)
📍 Affects 2 files
scripts/local-dev-setup.sh#L58-L59(this comment)scripts/local-dev-setup.sh#L79-L80scripts/local-dev-cleanup.sh#L11-L25
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/local-dev-setup.sh` around lines 58 - 59, Replace predictable /tmp
process-state paths with a per-user mode-0700 protected runtime directory,
preferably under XDG_RUNTIME_DIR, and ensure setup creates or validates it. In
scripts/local-dev-setup.sh lines 58-59, store Docling PID/log files there; in
scripts/local-dev-setup.sh lines 79-80, store Ollama PID/log files there; and in
scripts/local-dev-cleanup.sh lines 11-25, read and remove only those
corresponding protected files.
Source: Linters/SAST tools
| echo "Pulling embedding model..." | ||
| ollama pull nomic-embed-text:latest 2>/dev/null || true | ||
| ollama cp nomic-embed-text:latest nomic-ai/nomic-embed-text-v1.5 2>/dev/null || true | ||
| echo "✓ Embedding model ready" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not report the embedding model as ready after a failed install.
Both provisioning commands discard failures, so a network, disk, or daemon error still prints “Embedding model ready”; the controller can then start without its required embedding model. Fail setup here, or explicitly distinguish an optional alias failure from a successful pull.
Proposed fix
echo "Pulling embedding model..."
- ollama pull nomic-embed-text:latest 2>/dev/null || true
- ollama cp nomic-embed-text:latest nomic-ai/nomic-embed-text-v1.5 2>/dev/null || true
+ ollama pull nomic-embed-text:latest
+ ollama cp nomic-embed-text:latest nomic-ai/nomic-embed-text-v1.5
echo "✓ Embedding model ready"📝 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 "Pulling embedding model..." | |
| ollama pull nomic-embed-text:latest 2>/dev/null || true | |
| ollama cp nomic-embed-text:latest nomic-ai/nomic-embed-text-v1.5 2>/dev/null || true | |
| echo "✓ Embedding model ready" | |
| echo "Pulling embedding model..." | |
| ollama pull nomic-embed-text:latest | |
| ollama cp nomic-embed-text:latest nomic-ai/nomic-embed-text-v1.5 | |
| echo "✓ Embedding model ready" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/local-dev-setup.sh` around lines 88 - 91, Update the embedding model
provisioning block in the local setup script so a failed ollama pull does not
continue to the “Embedding model ready” message; propagate the pull failure and
stop setup. Treat the ollama cp alias operation separately, allowing only that
optional alias failure while still reporting readiness solely after a successful
nomic-embed-text:latest pull.
| INGESTION_BUCKET="$(yq -r '.spec.sourceCrawlerConfig.s3Config.bucket' "${SOURCE_CRAWLER_YAML}")" | ||
| DATA_STORAGE_BUCKET="$(yq -r '.spec.dataStorageBucket' "${CONTROLLER_CONFIG_YAML}")" | ||
| OUTPUT_BUCKET="$(yq -r '.spec.destinationSyncerConfig.s3DestinationConfig.bucket' "${DEST_SYNCER_YAML}")" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Defer bucket resolution to cmd_setup.
These command substitutions run for every command, so start, stop, and status fail before dispatch if yq is absent or a sample YAML is invalid—even though only setup needs the bucket names. Resolve and validate them inside cmd_setup after require_command yq.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/localstack.sh` around lines 20 - 22, Move the INGESTION_BUCKET,
DATA_STORAGE_BUCKET, and OUTPUT_BUCKET resolutions out of the top-level script
flow and into cmd_setup, after require_command yq runs. Keep these values
available to the setup logic, while allowing start, stop, and status to dispatch
without evaluating yq or validating the YAML files.
There was a problem hiding this comment.
kill localstack, move to the real deal 🙈
There was a problem hiding this comment.
use kube deployments for docling-serve and embeddings
5 participants
Add DEVELOPMENT.md guide covering the complete local development
workflow including LocalStack setup, CRD installation, controller
configuration, and e2e testing with required environment variables
Introduce make targets (local-dev-setup, local-dev-cleanup) with
corresponding shell scripts to automate local environment creation,
including S3 buckets, SQS queues, and Docling service setup.
Fix test-e2e target to pass KIND_CLUSTER and SKIP_CLUSTER_SETUP
environment variables, preventing duplicate cluster creation and
aligning local test execution with CI workflow.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes