Skip to content

chore: add download max retries parameter to environment variables#180

Merged
rtuszik merged 2 commits intortuszik:mainfrom
sEpt0r:main
Nov 12, 2025
Merged

chore: add download max retries parameter to environment variables#180
rtuszik merged 2 commits intortuszik:mainfrom
sEpt0r:main

Conversation

@sEpt0r
Copy link
Contributor

@sEpt0r sEpt0r commented Nov 1, 2025

What

This adds a new configuration parameter DOWNLOAD_MAX_RETRIES, which can be set via an environment variable instead of using the hardcoded limit of three download attempts.

Why

After testing in slower, “homelab-like” environments, I found that three attempts are sometimes not enough to complete the planet download. This change allows to increase the number of retry attempts by setting the environment variable in the container.

Copy link
Contributor

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

Hey there - I've reviewed your changes - here's some feedback:

  • Consider converting and validating DOWNLOAD_MAX_RETRIES as an integer in your config module (with a fallback or warning on invalid values) so that the rest of your code can assume the type is correct.
  • You may want to preserve the max_retries parameter in download_file’s signature (e.g. max_retries=None) to avoid breaking any existing calls or tests that rely on passing a custom retry count.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Consider converting and validating DOWNLOAD_MAX_RETRIES as an integer in your config module (with a fallback or warning on invalid values) so that the rest of your code can assume the type is correct.
- You may want to preserve the max_retries parameter in download_file’s signature (e.g. max_retries=None) to avoid breaking any existing calls or tests that rely on passing a custom retry count.

## Individual Comments

### Comment 1
<location> `src/downloader.py:505` </location>
<code_context>
-def download_file(url, destination, max_retries=3):
+def download_file(url, destination):
     start_time = time.time()
+    max_retries = int(config.DOWNLOAD_MAX_RETRIES)

     for attempt in range(max_retries):
</code_context>

<issue_to_address>
**suggestion:** Consider validating DOWNLOAD_MAX_RETRIES for non-integer or negative values.

Non-integer or negative values for DOWNLOAD_MAX_RETRIES may cause errors or unexpected behavior. Adding validation or a default fallback will improve robustness.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

def download_file(url, destination, max_retries=3):
def download_file(url, destination):
start_time = time.time()
max_retries = int(config.DOWNLOAD_MAX_RETRIES)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Consider validating DOWNLOAD_MAX_RETRIES for non-integer or negative values.

Non-integer or negative values for DOWNLOAD_MAX_RETRIES may cause errors or unexpected behavior. Adding validation or a default fallback will improve robustness.

Copy link
Owner

@rtuszik rtuszik left a comment

Choose a reason for hiding this comment

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

Thanks for this one as well. Totally forgot about this, my bad!

@rtuszik rtuszik merged commit 95e6fc8 into rtuszik:main Nov 12, 2025
8 checks passed
@binnichtaktiv
Copy link

After testing in slower, “homelab-like” environments, I found that three attempts are sometimes not enough to complete the planet download.

Thanks for this PR I had the exact same problem!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants