Skip to content

Conversation

@nemesifier
Copy link
Member

Added SKIP_FETCH argument.

@coderabbitai
Copy link

coderabbitai bot commented Jan 30, 2026

📝 Walkthrough

Walkthrough

The PR adds a skip-fetch optimization. Makefile: documents SKIP_FETCH and forwards it to build invocations. README: documents quick-rebuild workflow using SKIP_FETCH=1 and notes repositories must already exist. build.py: new --skip-fetch CLI flag and a skip_fetch parameter on clone_or_update_repo; when set, git clone/fetch/pull steps are skipped and the code proceeds to create symlinks only.

Sequence Diagram(s)

sequenceDiagram
participant Dev as Developer/Makefile
participant Builder as build.py
participant Repo as Git Remote
participant FS as Filesystem (repos & symlinks)

Dev->>Builder: invoke build with SKIP_FETCH=1
alt skip_fetch set
Builder->>FS: check repo path exists
FS-->>Builder: path exists
Builder->>FS: create/update symlink (no git ops)
else skip_fetch not set
Builder->>FS: check repo path exists
FS-->>Builder: path exists?
alt exists
Builder->>Repo: fetch/checkout/pull
Repo-->>Builder: updated repo
Builder->>FS: create/update symlink
else not exists
Builder->>Repo: clone repo
Repo-->>Builder: cloned
Builder->>FS: create symlink
end
end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main feature addition - implementing a SKIP_FETCH argument across the Makefile, README, and build.py files.
Description check ✅ Passed The description directly relates to the changeset by stating the SKIP_FETCH argument was added, matching the primary objective of the pull request.

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

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch features/skip-fetch

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between eaca85b and 2716e96.

📒 Files selected for processing (3)
  • Makefile
  • README.rst
  • build.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • README.rst
  • Makefile
🧰 Additional context used
🪛 Ruff (0.14.14)
build.py

[warning] 389-391: Avoid specifying long messages outside the exception class

(TRY003)

⏰ 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: build
🔇 Additional comments (5)
build.py (5)

333-391: Skip-fetch flow and missing-repo guard look solid.

Nice: git network calls are skipped when requested, and a missing repo fails fast with a clear error.


424-429: CLI flag addition is clear and well documented.


442-442: Comment-only tweak — no review needed.


463-474: Flag propagation to repo updates is complete.


269-271: No backward compatibility issue: all existing callers use keyword arguments exclusively. The parameter ordering does not break any calls within this codebase.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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

@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.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
build.py (1)

287-335: ⚠️ Potential issue | 🟡 Minor

Fail fast when --skip-fetch is used without a local repo.

Right now skip_fetch=True can create a symlink to a non‑existent path; the build then fails later with less clear errors. Consider validating the repo exists and exit with a clear message.

💡 Suggested fix
     clone_path = os.path.abspath(os.path.join("modules", dir_name))
+    if skip_fetch and not os.path.exists(clone_path):
+        raise SystemExit(
+            f"SKIP_FETCH is set but repository '{name}' is missing at {clone_path}. "
+            "Run a normal build once to clone repositories."
+        )
     if os.path.exists(clone_path) and not skip_fetch:
🤖 Fix all issues with AI agents
In `@README.rst`:
- Around line 43-45: Remove the trailing space at the end of the code line "make
build VERSION=dev FORMATS=html " and unindent the indented continuation under
the Note paragraph so the note text is not treated as a code block; apply the
same fixes to the other occurrence (the block at lines 187-189) to eliminate the
reStructuredText formatter failure.
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1696a03 and 7bb0ad2.

📒 Files selected for processing (3)
  • Makefile
  • README.rst
  • build.py
🧰 Additional context used
🪛 GitHub Actions: OpenWISP Docs
README.rst

[error] 1-1: ReStructuredText formatting check failed. README.rst could be reformatted. 1 out of 44 files could be reformatted.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

@github-project-automation github-project-automation bot moved this from To do (general) to In progress in OpenWISP Contributor's Board Jan 30, 2026
@nemesifier nemesifier force-pushed the features/skip-fetch branch 2 times, most recently from 21c6fc2 to eaca85b Compare January 30, 2026 22:41
coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 30, 2026
Copy link

@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.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
build.py (1)

289-343: ⚠️ Potential issue | 🟡 Minor

Fail fast when --skip-fetch is used without a local repo.
With skip_fetch=True and a missing modules/<dir_name>, the code will create a broken symlink and later fail in Sphinx. Consider raising a clear error up front.

💡 Suggested guard
     clone_path = os.path.abspath(os.path.join("modules", dir_name))
     if os.path.exists(clone_path) and not skip_fetch:
         print(f"Repository '{name}' already exists. Updating...")
         subprocess.run(
@@
     elif not skip_fetch:
         print(f"Cloning repository '{name}'...")
         subprocess.run(
@@
             check=True,
         )
+    elif not os.path.exists(clone_path):
+        raise FileNotFoundError(
+            f"Repository '{name}' not found at {clone_path}; run without --skip-fetch first."
+        )
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7bb0ad2 and eaca85b.

📒 Files selected for processing (3)
  • Makefile
  • README.rst
  • build.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • Makefile
  • README.rst
🧰 Additional context used
🪛 Ruff (0.14.14)
build.py

[error] 302-308: Starting a process with a partial executable path

(S607)

⏰ 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: build
🔇 Additional comments (3)
build.py (3)

268-276: Clear skip_fetch API + docstring update.
Nice, the new parameter is documented in the right place and the intent is clear.


293-308: refs/heads/ checkout drops tag support.
The new refs/heads/{branch} fetch/checkout will fail if branch is a tag (the comment about detached HEAD suggests tags are supported). If tags are still expected, handle refs/tags/ or allow fully-qualified refs; otherwise update docs to make “branch-only” explicit.


367-416: --skip-fetch CLI wiring looks consistent end-to-end.
Argument definition and propagation into all clone/update calls are aligned.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

@nemesifier nemesifier merged commit 2716e96 into master Jan 31, 2026
3 checks passed
@nemesifier nemesifier deleted the features/skip-fetch branch January 31, 2026 15:29
@github-project-automation github-project-automation bot moved this from In progress to Done in OpenWISP Contributor's Board Jan 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Development

Successfully merging this pull request may close these issues.

2 participants