Skip to content

dockerize fetch-nest-dump to avoid local dependency on poetry #4581

Draft
devnchill wants to merge 2 commits intoOWASP:mainfrom
devnchill:enhancement/dockerize-fetch-nest-dump
Draft

dockerize fetch-nest-dump to avoid local dependency on poetry #4581
devnchill wants to merge 2 commits intoOWASP:mainfrom
devnchill:enhancement/dockerize-fetch-nest-dump

Conversation

@devnchill
Copy link
Copy Markdown

@devnchill devnchill commented Apr 23, 2026

Proposed change

Resolves #4536

Add the PR description here.

  • runs fetch-nest-dump.py inside nest-backend container
  • stores the nest.dump file in backend/data to be later used for storing/updating data in databases

Checklist

  • Required: I followed the contributing workflow
  • Required: I verified that my code works as intended and resolves the issue as described
  • Required: I ran make check-test locally: all warnings addressed, tests passed
  • I used AI for code, documentation, tests, or communication related to this PR

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 23, 2026

Walkthrough

The fetch-nest-dump Makefile target now runs the Python module inside the backend Docker Compose service (nest-local) instead of via poetry run on the host. After execution it adjusts backend/data permissions and prints a directory listing for verification.

Changes

Cohort / File(s) Summary
Makefile Docker Containerization
backend/Makefile
Updated fetch-nest-dump target to run the fetch command within Docker Compose (-f docker-compose/local/compose.yaml --project-name nest-local exec backend ...) rather than poetry run on host; added chmod on backend/data and an ls -l backend/data output step.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

docker

Suggested reviewers

  • arkid15r
  • kasya
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Out of Scope Changes check ❓ Inconclusive The Makefile change appears related to the objective, but the description mentions backend/data permission changes and backend/dump file storage that are not evident in the provided summary. Clarify whether permission changes and dump directory handling are included in this PR or if they belong to other commits.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: dockerizing the fetch-nest-dump command to eliminate local Poetry dependency.
Linked Issues check ✅ Passed The PR addresses issue #4536 by dockerizing fetch-nest-dump to run inside the container, eliminating the need for local Poetry installation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed The PR description clearly relates to the changeset, explaining that it runs fetch-nest-dump.py inside the nest-backend container and stores the nest.dump file in backend/data.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.

Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.

👉 Steps to fix this

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@backend/Makefile`:
- Around line 123-130: The fetch-nest-dump Makefile target should be fixed:
remove the commented-out legacy python command, stop running a host chmod with
unsafe mode 757 (either ensure ownership in Dockerfile.local or if kept, guard
it with a directory existence check and use 775), replace the backticked string
around nest-backend (which triggers command substitution) with a literal name or
drop quotes, call docker compose with the same compose file flag used elsewhere
(e.g., -f docker-compose/local/compose.yaml) or reuse the exec-backend-command
pattern instead of relying solely on --project-name, and remove the debug ls -l
backend/data or replace it with a concise success echo; also review
upload-nest-dump to make its poetry invocation consistent (dockerized) or
document why it differs.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 30a39de3-f78a-4d5f-9908-cd3eb8802901

📥 Commits

Reviewing files that changed from the base of the PR and between 131615e and b0c7a5b.

📒 Files selected for processing (1)
  • backend/Makefile

Comment thread backend/Makefile
Comment on lines 123 to +130
fetch-nest-dump: ## Download backend/data/nest.dump from S3 if the remote copy changed
@cd backend && poetry run python -m scripts.fetch_nest_dump
# @cd backend && poetry run python -m scripts.fetch_nest_dump
@echo "changing permission of /backend/data"
@chmod 757 backend/data
@echo "fetching data inside docker container `nest-backend`"
@docker compose --project-name nest-local exec backend poetry run python -m scripts.fetch_nest_dump
@echo "Check if it worked"
@ls -l backend/data
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Several issues in the reworked fetch-nest-dump target.

  1. Line 126 — chmod 757 is world-writable and runs on the host. Mode 757 grants rwx to others, which is a needless privilege grant on the developer's host filesystem. The proper fix is to ensure the owasp user in the backend image owns /home/owasp/data (via Dockerfile.local), not to loosen host permissions at every invocation. At minimum use 775. Also, because this runs on the host, it requires that the invoking host user has permission to chmod that directory (won't work if it was previously created as root by Docker).

  2. Line 127 — backticks around `nest-backend` trigger command substitution. make passes the line to the shell, which will try to execute nest-backend and fail with nest-backend: command not found, polluting output. Use single quotes or drop the backticks.

  3. Line 124 — remove commented-out legacy command rather than leaving it as dead code.

  4. Line 128 — missing compose file reference. Every other docker compose invocation in this Makefile passes -f docker-compose/<env>/compose.yaml (see run-backend-e2e, run-backend-fuzz, test-fuzz). Relying solely on --project-name nest-local requires the nest-backend container to already be running and depends on docker's project-name resolution. If the stack isn't up, this fails with a confusing error. Consider either using docker exec -i nest-backend … (consistent with exec-backend-command) or passing -f docker-compose/local/compose.yaml.

  5. Line 130 — ls -l backend/data looks like a debug leftover. Either drop it or replace with a meaningful success message.

  6. Consistency gap with upload-nest-dump (line 104). The PR's stated goal is to remove the local poetry dependency, but upload-nest-dump still runs poetry run on the host. Consider the same dockerized approach there for consistency, or at least document why it's intentionally different.

🛠️ Proposed fix
 fetch-nest-dump: ## Download backend/data/nest.dump from S3 if the remote copy changed
-	# `@cd` backend && poetry run python -m scripts.fetch_nest_dump
-	`@echo` "changing permission of /backend/data"
-	`@chmod` 757 backend/data
-	`@echo` "fetching data inside docker container `nest-backend`"
-	`@docker` compose --project-name nest-local exec backend poetry run python -m scripts.fetch_nest_dump
-	`@echo` "Check if it worked"
-	`@ls` -l backend/data
+	`@echo` "Fetching nest.dump inside the nest-backend container"
+	`@CMD`="poetry run python -m scripts.fetch_nest_dump" $(MAKE) exec-backend-command

(Preferred: fix directory ownership in docker/backend/Dockerfile.local so no host chmod is needed. If a host-side chmod must stay, use 775 and guard it with [ -d backend/data ] && chmod 775 backend/data || true.)

🧰 Tools
🪛 checkmake (0.3.2)

[warning] 123-123: Target body for "fetch-nest-dump" exceeds allowed length of 5 lines (7).

(maxbodylength)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/Makefile` around lines 123 - 130, The fetch-nest-dump Makefile target
should be fixed: remove the commented-out legacy python command, stop running a
host chmod with unsafe mode 757 (either ensure ownership in Dockerfile.local or
if kept, guard it with a directory existence check and use 775), replace the
backticked string around nest-backend (which triggers command substitution) with
a literal name or drop quotes, call docker compose with the same compose file
flag used elsewhere (e.g., -f docker-compose/local/compose.yaml) or reuse the
exec-backend-command pattern instead of relying solely on --project-name, and
remove the debug ls -l backend/data or replace it with a concise success echo;
also review upload-nest-dump to make its poetry invocation consistent
(dockerized) or document why it differs.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 1 file

Confidence score: 2/5

  • There are two high-confidence issues in backend/Makefile, including a security-sensitive permission setting, so this carries elevated merge risk.
  • The chmod 757 on backend/data is the most severe concern: world-writable permissions are unnecessarily broad and can expose the host filesystem to misuse; tightening to safer permissions/ownership is recommended before merge.
  • Backticks around nest-backend in backend/Makefile will trigger shell command substitution, which can produce command not found errors and break expected script behavior.
  • Pay close attention to backend/Makefile - fix unsafe directory permissions and replace backticks to avoid shell substitution failures.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="backend/Makefile">

<violation number="1" location="backend/Makefile:126">
P1: `chmod 757` grants world-writable and world-executable permissions (`rwxr-xrwx`) to `backend/data`, which is an unnecessary security risk on the host filesystem. Use `775` at minimum, or better yet, fix directory ownership inside the Docker image (e.g., in `Dockerfile.local`) so no host-side `chmod` is needed.</violation>

<violation number="2" location="backend/Makefile:127">
P2: Backticks around `nest-backend` will be interpreted as shell command substitution. The shell will try to execute `nest-backend` as a command (which doesn't exist), producing a "command not found" error and garbled output. Use single quotes or remove the backticks entirely.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread backend/Makefile Outdated
Comment thread backend/Makefile Outdated
@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 issues found across 1 file (changes from recent commits).

Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.

@devnchill
Copy link
Copy Markdown
Author

@arkid15r in docs/Makefile , for update-docs-dependencies target, it runs

	cd docs && poetry update

since aim of our issue initially was to bypass poetry overall locally , i am wondering if i should work on dockerizing poetry for updating docs as well or should this pr just be focused on fetch-nest-dump ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add poetry as pre requisite

1 participant