feat(jans-linux-setup): fallback to env var in case downloading fails - #14632
Conversation
Signed-off-by: Mustafa Baser <mbaser@mail.com>
devrimyatar
added
the
kind-enhancement
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe ChangesDownload fallback handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@jans-linux-setup/jans_setup/setup_app/utils/base.py`:
- Around line 413-416: Update the download flow surrounding urlretrieve so
downloads write to a temporary file and atomically replace dst only after
successful completion. Ensure failed downloads clean up the temporary file and
never leave or reuse a partial destination; preserve the existing
os.path.exists(dst) fallback only for genuinely pre-existing files.
- Around line 395-398: Update the retry handling around download_tries so it
increments and checks the attempt count before logging a retry or calling
time.sleep. Only log the retry and delay when another download attempt will
actually run; on the final failure, proceed directly to the fallback path.
- Around line 394-398: Update the exception handling in the download retry logic
around the download function to catch only expected failures such as
urllib.error.URLError and OSError, log the caught exception details with the
retry message, and allow unexpected exceptions—including KeyboardInterrupt and
SystemExit—to propagate without retrying.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bdd1dfd7-95cf-4b60-baf8-4473b2734cab
📒 Files selected for processing (1)
jans-linux-setup/jans_setup/setup_app/utils/base.py
Signed-off-by: Mustafa Baser <mbaser@mail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
jans-linux-setup/jans_setup/setup_app/utils/base.py (2)
419-420: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAtomically install the environment fallback.
shutil.copy(src, dst)writes directly to the destination. A copy failure can leavedsttruncated or partial, which later calls may treat as a valid existing file. Copy to a temporary file in the destination directory, then useos.replace()after success and clean up on 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 `@jans-linux-setup/jans_setup/setup_app/utils/base.py` around lines 419 - 420, Update the copy operation following the “Copying” log to write src into a temporary file in dst’s directory, then atomically install it with os.replace() only after the copy succeeds. Ensure the temporary file is cleaned up on both success and failure, while preserving the existing destination path and logging behavior.
413-417: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReset the global opener on the early return.
The same-file fallback returns before
urllib.request.install_opener(None)runs. In a long-lived process, subsequent downloads inherit the previous opener and headers, potentially leaking custom authentication headers. Move opener cleanup into afinallyblock covering all return and exit paths.🤖 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 `@jans-linux-setup/jans_setup/setup_app/utils/base.py` around lines 413 - 417, Update the fallback copy flow around the same-file check in the relevant utility function so urllib.request.install_opener(None) executes in a finally block covering every return and exit path, including the early return when src and dst refer to the same file. Preserve the existing fallback behavior while ensuring no custom opener or authentication headers remain installed.
🤖 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 `@jans-linux-setup/jans_setup/setup_app/utils/base.py`:
- Line 393: Update the download-size log in the relevant setup utility to use an
f-string instead of str.format, while preserving the existing
os.path.getsize(dst) value and message.
- Around line 390-396: Update the download retry logic around urlretrieve and
dst_tmp_fn so every attempt removes the temporary file in a finally block,
including failed or partial downloads, while preserving the existing successful
move to dst and retry behavior.
- Around line 402-404: Update the exception handler around mylog to interpolate
the caught exception into the message using an f-string, and correct “contuinue”
to “continue”; preserve the existing sys.exit(2) behavior.
---
Outside diff comments:
In `@jans-linux-setup/jans_setup/setup_app/utils/base.py`:
- Around line 419-420: Update the copy operation following the “Copying” log to
write src into a temporary file in dst’s directory, then atomically install it
with os.replace() only after the copy succeeds. Ensure the temporary file is
cleaned up on both success and failure, while preserving the existing
destination path and logging behavior.
- Around line 413-417: Update the fallback copy flow around the same-file check
in the relevant utility function so urllib.request.install_opener(None) executes
in a finally block covering every return and exit path, including the early
return when src and dst refer to the same file. Preserve the existing fallback
behavior while ensuring no custom opener or authentication headers remain
installed.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ce9b438a-7883-4098-b8fd-5dec4575fecf
📒 Files selected for processing (1)
jans-linux-setup/jans_setup/setup_app/utils/base.py
Signed-off-by: Mustafa Baser <mbaser@mail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jans-linux-setup/jans_setup/setup_app/utils/base.py (1)
419-421: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestore the default urllib opener before returning from the same-file case.
The same-file fallback returns before
urllib.request.install_opener(None), leaving the temporary opener globally installed for later urllib calls. Log the skip, then fall through to teardown.🤖 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 `@jans-linux-setup/jans_setup/setup_app/utils/base.py` around lines 419 - 421, Update the same-file branch in the relevant fallback utility to log the skip without returning immediately, allowing execution to reach urllib.request.install_opener(None) and restore the default opener. Preserve the existing same-file detection and message, and ensure normal fallback work is skipped while teardown still runs.
🤖 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.
Outside diff comments:
In `@jans-linux-setup/jans_setup/setup_app/utils/base.py`:
- Around line 419-421: Update the same-file branch in the relevant fallback
utility to log the skip without returning immediately, allowing execution to
reach urllib.request.install_opener(None) and restore the default opener.
Preserve the existing same-file detection and message, and ensure normal
fallback work is skipped while teardown still runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8801f70b-8205-4c17-b8b7-4ecd6311330f
📒 Files selected for processing (1)
jans-linux-setup/jans_setup/setup_app/utils/base.py
Signed-off-by: Mustafa Baser <mbaser@mail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jans-linux-setup/jans_setup/setup_app/utils/base.py (1)
423-424: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCopy the fallback atomically.
shutil.copy(src, dst)truncatesdstbefore copying. A read or disk failure can replace a valid existing artifact with a partial file; later non-forced installer runs skip downloads when that path exists and may consume it. Copy to a same-directory temporary file, thenos.replace()it into place.Proposed fix
- shutil.copy(src, dst) + try: + shutil.copy(src, dst_tmp_fn) + os.replace(dst_tmp_fn, dst) + finally: + if os.path.exists(dst_tmp_fn): + os.remove(dst_tmp_fn)🤖 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 `@jans-linux-setup/jans_setup/setup_app/utils/base.py` around lines 423 - 424, Update the fallback copy operation around the shutil.copy call to write src into a temporary file in dst’s directory, then atomically commit it with os.replace only after the copy succeeds; clean up the temporary file on failure while preserving the existing logging and destination behavior.
🤖 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.
Outside diff comments:
In `@jans-linux-setup/jans_setup/setup_app/utils/base.py`:
- Around line 423-424: Update the fallback copy operation around the shutil.copy
call to write src into a temporary file in dst’s directory, then atomically
commit it with os.replace only after the copy succeeds; clean up the temporary
file on failure while preserving the existing logging and destination behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 833ad086-80a5-4d2a-a130-157dca37f0c0
📒 Files selected for processing (1)
jans-linux-setup/jans_setup/setup_app/utils/base.py
Signed-off-by: Mustafa Baser <mbaser@mail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jans-linux-setup/jans_setup/setup_app/utils/base.py (1)
419-421: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winGuard the installed urllib opener on fallback exit paths.
download()installs a process-global opener with caller-provided headers, but the fallbackreturnandsys.exit(2)paths bypassurllib.request.install_opener(None). Wrap the download/fallback logic intry/finallyso later urllib calls don’t inherit this opener and headers.🤖 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 `@jans-linux-setup/jans_setup/setup_app/utils/base.py` around lines 419 - 421, Update download() to wrap the opener installation and all download/fallback logic in a try/finally block, ensuring urllib.request.install_opener(None) runs on every exit path, including the existing same-file return and sys.exit(2) failure path. Preserve the current fallback behavior and logging.
♻️ Duplicate comments (1)
jans-linux-setup/jans_setup/setup_app/utils/base.py (1)
423-424: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake the environment-file fallback atomic.
shutil.copy(src, dst)writes directly to the final destination. If the copy is interrupted or fails,dstcan remain truncated and be treated as a valid existing file on a later run. Copy to a temporary file in the destination directory, then atomically replacedstafter success.🤖 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 `@jans-linux-setup/jans_setup/setup_app/utils/base.py` around lines 423 - 424, Update the fallback copy operation in the surrounding utility method to write src to a temporary file in dst’s directory, then atomically replace dst only after the copy succeeds. Ensure temporary-file cleanup on failure and preserve the existing logging and destination behavior.
🤖 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 `@jans-linux-setup/jans_setup/setup_app/utils/base.py`:
- Around line 396-401: Update the URLError handling around the download retry
logic so the log message accurately reflects the remaining retries: do not
report a retry when download_tries reaches the final attempt, and ensure the
first failure’s message matches the actual number of attempts remaining. Keep
the existing retry guard and fallback behavior unchanged.
- Line 413: Update the env_var sanitization expression to use ASCII-only word
matching, preserving alphanumeric characters plus underscores, periods, and
hyphens while replacing Unicode characters such as “é” with underscores.
---
Outside diff comments:
In `@jans-linux-setup/jans_setup/setup_app/utils/base.py`:
- Around line 419-421: Update download() to wrap the opener installation and all
download/fallback logic in a try/finally block, ensuring
urllib.request.install_opener(None) runs on every exit path, including the
existing same-file return and sys.exit(2) failure path. Preserve the current
fallback behavior and logging.
---
Duplicate comments:
In `@jans-linux-setup/jans_setup/setup_app/utils/base.py`:
- Around line 423-424: Update the fallback copy operation in the surrounding
utility method to write src to a temporary file in dst’s directory, then
atomically replace dst only after the copy succeeds. Ensure temporary-file
cleanup on failure and preserve the existing logging and destination behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cd80a0a8-3fb8-409f-bbcd-2171bc26cd29
📒 Files selected for processing (1)
jans-linux-setup/jans_setup/setup_app/utils/base.py
Signed-off-by: Mustafa Baser <mbaser@mail.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Mustafa Baser <mbaser@mail.com>
Signed-off-by: Mustafa Baser <mbaser@mail.com>
Signed-off-by: Mustafa Baser <mbaser@mail.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
jans-linux-setup/jans_setup/setup_app/utils/base.py (2)
385-429: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd regression coverage for the retry and fallback state machine.
Test successful moves, three
URLErrorattempts, temporary-file cleanup, environment-source copying, same-file fallback, existing-destination fallback, and missing-source exit behavior.🤖 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 `@jans-linux-setup/jans_setup/setup_app/utils/base.py` around lines 385 - 429, Add regression tests for the download/retry and fallback flow surrounding urllib.request.urlretrieve and the fallback environment-variable logic. Cover successful download and move, three URLError attempts, temporary-file cleanup, copying from the configured environment source, same-file fallback, existing-destination fallback, and exiting when neither the source nor destination exists; use mocks and temporary paths to isolate filesystem, timing, and process-exit behavior.
383-393: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore the global opener on every exit path.
urllib.request.install_opener()changes process-global state, but the same-file fallback returns before Line 431, and thesys.exit(2)paths bypass it as well. Wrap the installed-opener scope in an outertry/finallyso later URL operations cannot inherit this downloader’s opener.Also applies to: 402-404, 417-421, 427-431
🤖 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 `@jans-linux-setup/jans_setup/setup_app/utils/base.py` around lines 383 - 393, Update the downloader function containing urllib.request.install_opener(opener) to wrap all subsequent download, fallback-return, and sys.exit paths in an outer try/finally, restoring the prior global urllib opener in finally. Ensure restoration occurs on every exit path while preserving the existing download behavior.
🤖 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.
Outside diff comments:
In `@jans-linux-setup/jans_setup/setup_app/utils/base.py`:
- Around line 385-429: Add regression tests for the download/retry and fallback
flow surrounding urllib.request.urlretrieve and the fallback
environment-variable logic. Cover successful download and move, three URLError
attempts, temporary-file cleanup, copying from the configured environment
source, same-file fallback, existing-destination fallback, and exiting when
neither the source nor destination exists; use mocks and temporary paths to
isolate filesystem, timing, and process-exit behavior.
- Around line 383-393: Update the downloader function containing
urllib.request.install_opener(opener) to wrap all subsequent download,
fallback-return, and sys.exit paths in an outer try/finally, restoring the prior
global urllib opener in finally. Ensure restoration occurs on every exit path
while preserving the existing download behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 710525b7-f371-492a-a329-7051d207ce29
📒 Files selected for processing (1)
jans-linux-setup/jans_setup/setup_app/utils/base.py
|
4 participants
Prepare
Description
Target issue
closes #14631
Implementation Details
Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Summary by CodeRabbit