med9110 commented on code in PR #113:
URL: https://github.com/apache/solr-orbit/pull/113#discussion_r4293870662
##########
solrorbit/builder/solr_provisioner.py:
##########
@@ -136,14 +138,29 @@ def download(self, version: str) -> str:
for base_url in (_DOWNLOADS_URL, _ARCHIVE_9_URL, _ARCHIVE_PRE9_URL):
url = f"{base_url}/{version}/{tarball}"
logger.info("Downloading Solr %s from %s", version, url)
+
+ temp_path = None
+
try:
- urllib.request.urlretrieve(url, dest)
+ fd, temp_path = tempfile.mkstemp(
+ dir=self.cache_dir,
+ prefix=f"{tarball}.",
+ suffix=".tmp"
+ )
+ os.close(fd)
+ try:
+ urllib.request.urlretrieve(url, temp_path)
+ except Exception as exc:
+ logger.warning("Download from %s failed: %s", url, exc)
+ continue
+ os.replace(temp_path, dest)
Review Comment:
Thanks @janhoy, good catch!
I've addressed this in `bb2e522c` by switching from `mkstemp()` to
`TemporaryDirectory()`, allowing `urlretrieve()` to create the archive with
permissions respecting the system's `umask`, rather than forcing `0600`.
The download still uses `os.replace()` for atomic publication, and the
temporary directory is cleaned up on success or failure.
I also added a permissions regression test and strengthened the cleanup
assertions.
Verified locally on Python 3.13: **1,146 passed, 5 skipped**, and Ruff
checks passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
