SG-41115 more changes

[julien-lang]

TODO

[Copilot]

Pull Request Overview

This PR refactors the retry logic for RPC calls and data uploads in the Shotgun API client. The changes centralize retry configuration by removing hardcoded constants and using configuration-based retry parameters instead.

Key changes:

• Replace hardcoded MAX_ATTEMPTS and BACKOFF constants with configurable max_rpc_attempts and rpc_attempt_interval from the config object
• Standardize retry loop implementation across _call_rpc, _make_call, _upload_data_to_storage, and _send_form methods
• Add SSL error handling with detailed logging in _upload_data_to_storage

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The sleep duration calculation multiplies attempt * rpc_attempt_interval, but attempt starts at 0 and increments to 1 before the first iteration's logic. This means the first retry (after a failure) will sleep for 1 * rpc_attempt_interval, the second retry for 2 * rpc_attempt_interval, etc. However, in the old code, the sleep was attempt * BACKOFF where attempt started at 1, giving the same progression. The issue is that when attempt reaches max_rpc_attempts, the loop exits, but the last attempt number logged will be max_rpc_attempts. If max_rpc_attempts is 3, you'll see attempts 1, 2, 3, but only 3 actual iterations (0->1, 1->2, 2->3 before exit). This is correct behavior, but the new loop condition attempt < max_rpc_attempts with attempt starting at 0 means if max_rpc_attempts is 3, you get attempts numbered 1, 2, 3 (three attempts total), which matches the old behavior where MAX_ATTEMPTS = 3 gave attempts 1, 2, 3. No issue here on second thought, but worth verifying the intent is the same number of attempts.

Suggested change

	attempt = 0
	while attempt < max_rpc_attempts:
	if attempt:
	time.sleep(attempt * rpc_attempt_interval)
	attempt += 1
	for attempt in range(1, max_rpc_attempts + 1):
	if attempt > 1:
	time.sleep((attempt - 1) * rpc_attempt_interval)

[Copilot]

Inconsistent loop condition: this uses attempt <= max_rpc_attempts while other retry loops in the same file use attempt < max_rpc_attempts (lines 3762, 3925, 4655). If max_rpc_attempts is 3, this loop will execute 4 times (attempt values 1, 2, 3, 4), while the others execute 3 times. This should be attempt < max_rpc_attempts for consistency.

Suggested change

	while attempt <= max_rpc_attempts:
	while attempt < max_rpc_attempts:

[Copilot]

Corrected spelling of 'unitests' to 'unittests'.

Suggested change

	## TODO - add unitests for those cases
	## TODO - add unittests for those cases

[Copilot]

This TODO comment is unclear about what 'create only one attempt' means. Consider rephrasing to something like 'TODO: Skip retries for SSL errors' or 'TODO: Fail immediately on SSL errors without retrying' to clarify the intended behavior.

Suggested change

	# TODO create only one attempt for SSL errors.
	# TODO: Fail immediately on SSL errors without retrying.

[Copilot]

The safe_short_url function lacks a docstring. Given its purpose of sanitizing URLs for logging (removing credentials and truncating paths), a docstring would improve maintainability by explaining the security and formatting rationale.

Suggested change

	def safe_short_url(url: str, max_path_length: int = 80) -> str:
	def safe_short_url(url: str, max_path_length: int = 80) -> str:
	"""
	Sanitize a URL for safe logging by removing sensitive information and truncating long paths.
	This function removes credentials (such as HTTP Basic Auth usernames/passwords and API keys)
	from the URL, strips query parameters and fragments, and truncates the path if it exceeds
	`max_path_length` by replacing the middle part with '[...]'. This helps prevent accidental
	exposure of sensitive data in logs or error messages.
	Args:
	url (str): The URL to sanitize.
	max_path_length (int): Maximum allowed length for the path component. Defaults to 80.
	Returns:
	str: The sanitized, shortened URL safe for logging.
	"""