SG-40996 prevent tk component autoupdate #1077
Conversation
…inimum_python_version not met - Added _check_minimum_python_version() method in base.py - Updated appstore.py get_latest_version() to check Python compatibility - If latest version requires newer Python, auto-update is blocked and current version is retained - Uses tank.util.version.is_version_newer_or_equal() for robust version comparison
… sg_matching_records for filtering, reduce nesting.
julien-lang
left a comment
julien-lang
left a comment
There was a problem hiding this comment.
I'm surprised to see so much new code in python/tank/descriptor/io_descriptor/appstore.py. We already have some code checking compatibility from app store components right? Like core min version, SG min version, ...
Could you write some tests too please? Like a test showing a component version is not selected because of the current Python version. But then running a higher Python version, the component get picked up
- 3 unit tests for _check_minimum_python_version validation - 2 end-to-end tests demonstrating version selection with cache fallback - Tests verify that incompatible versions are detected and older compatible versions are used when available
eduardoChaucaGallegos
requested review from
carlos-villavicencio-adsk and
julien-lang
| try: | ||
| tags = [x["name"] for x in metadata["sg_version_data"]["tags"]] | ||
| if self.__match_label(tags): | ||
| version_numbers.append(version_str) |
There was a problem hiding this comment.
Can we yield instead and return an iterator instead of building a list object?
Also can you add typing info to this method and the other new ones below?
| tags = [x["name"] for x in metadata["sg_version_data"]["tags"]] | ||
| if self.__match_label(tags): | ||
| version_numbers.append(version_str) | ||
| except Exception as e: |
There was a problem hiding this comment.
Do we know which specific exception we are expecting here?
| try: | ||
| sorted_versions = [] | ||
| for v in version_numbers: | ||
| inserted = False | ||
| for i, existing in enumerate(sorted_versions): | ||
| if is_version_newer(v, existing): | ||
| sorted_versions.insert(i, v) | ||
| inserted = True | ||
| break | ||
| if not inserted: | ||
| sorted_versions.append(v) | ||
| return sorted_versions |
There was a problem hiding this comment.
Can we use sorted with a key parameter instead here?
I think we do that already in core dcc version comparison
| :returns: Tuple of (is_compatible, descriptor) where descriptor is the | ||
| IODescriptorAppStore instance if compatible, None otherwise |
There was a problem hiding this comment.
I don't like returning tuples like these. What if we return either the IODescriptor or None if not found? It's basically the same thing without an extra boolean parameter
| ) | ||
| return (False, None) | ||
|
|
||
| except Exception as e: |
There was a problem hiding this comment.
Specific exception here I think
| return descriptor | ||
|
|
||
| log.debug("No compatible cached version found") | ||
| return None |
There was a problem hiding this comment.
No need for a return statement at the end of the method. This is Python's default behaviour
| return None |
|
|
||
| else: | ||
| # no constraints applied. Pick first (latest) match | ||
| # no constraints applied. Check if latest version is Python compatible |
There was a problem hiding this comment.
Could we make this code less "Python version" specific and more generic for any current or future "requirements" we want to have?
| except Exception as e: | ||
| log.warning( | ||
| f"Could not check Python compatibility for {version_to_use}: {e}. Proceeding with auto-update." | ||
| ) |
There was a problem hiding this comment.
Again, can we have a list of expected exception instead?
| # Get current Python version as string (e.g., "3.9.13") | ||
| current_version_str = ( | ||
| f"{sys.version_info[0]}.{sys.version_info[1]}.{sys.version_info[2]}" | ||
| ) |
There was a problem hiding this comment.
Would that be easier to handle tuple versions instead?
| # Manifest requiring Python 3.10 should be incompatible with Python 3.9 | ||
| manifest_requiring_310 = {"minimum_python_version": "3.10"} | ||
| self.assertFalse(io_desc._check_minimum_python_version(manifest_requiring_310)) | ||
|
|
||
| # Manifest requiring Python 3.7 should be compatible with Python 3.9 | ||
| manifest_requiring_37 = {"minimum_python_version": "3.7"} | ||
| self.assertTrue(io_desc._check_minimum_python_version(manifest_requiring_37)) | ||
|
|
||
| # Manifest without requirement should be compatible | ||
| manifest_no_requirement = {} | ||
| self.assertTrue(io_desc._check_minimum_python_version(manifest_no_requirement)) |
There was a problem hiding this comment.
Remove useless variables
| # Manifest requiring Python 3.10 should be incompatible with Python 3.9 | |
| manifest_requiring_310 = {"minimum_python_version": "3.10"} | |
| self.assertFalse(io_desc._check_minimum_python_version(manifest_requiring_310)) | |
| # Manifest requiring Python 3.7 should be compatible with Python 3.9 | |
| manifest_requiring_37 = {"minimum_python_version": "3.7"} | |
| self.assertTrue(io_desc._check_minimum_python_version(manifest_requiring_37)) | |
| # Manifest without requirement should be compatible | |
| manifest_no_requirement = {} | |
| self.assertTrue(io_desc._check_minimum_python_version(manifest_no_requirement)) | |
| # Manifest requiring Python 3.10 should be incompatible with Python 3.9 | |
| self.assertFalse( | |
| io_desc._check_minimum_python_version( | |
| {"minimum_python_version": "3.10"} | |
| ) | |
| ) | |
| # Manifest requiring Python 3.7 should be compatible with Python 3.9 | |
| self.assertTrue( | |
| io_desc._check_minimum_python_version( | |
| {"minimum_python_version": "3.7"} | |
| ) | |
| ) | |
| # Manifest without requirement should be compatible | |
| self.assertTrue( | |
| io_desc._check_minimum_python_version({}) | |
| ) |
Description
This pull request introduces Python version compatibility checks into the App Store descriptor logic, ensuring that bundles are only auto-updated to versions compatible with the current Python interpreter. It also adds utility methods for filtering and sorting cached versions and improves error handling and logging throughout the descriptor code.
Python version compatibility enforcement:
Added a
_check_minimum_python_versionmethod inIODescriptorBaseto compare the current Python version with the bundle'sminimum_python_versionrequirement from its manifest. This method is now used to block auto-updates to incompatible versions and to select the highest compatible cached version if the latest is not suitable. [1] [2]Updated
get_latest_versioninIODescriptorAppStoreto check Python compatibility before selecting the latest version, falling back to the newest compatible cached version if necessary, and logging appropriate warnings or info.Cached version selection and filtering:
IODescriptorAppStorefor filtering cached versions by label, App Store availability, and Python compatibility, and for sorting versions in descending order. These methods improve the robustness and maintainability of version selection logic.Imports and code cleanup:
io_descriptor/appstore.pyandio_descriptor/base.pyfor consistency and to support new functionality (e.g., importingsysandis_version_newer_or_equal). [1] [2]