-
Notifications
You must be signed in to change notification settings - Fork 129
Conversation
This would require deploying new executables/installers, yes? :) Were you able to reproduce the original issue of it trying to do a force install in headless mode? And did this avoid it on your end? Just wondering how "safe" it is for me to just throw this PR into production to see if it'd would fix the issue or whether this might do more harm. |
This still needs little refactor to move the compatibility check out... |
yes, this will need new build
I haven't reproduced it, but find similar issue with headless mode that lead me to this. The re-install logic is sound I think, problem is that it can't find the latest version correctly so it is trying to reinstall it. But please wait with deployment until the compatibility check is refactored. |
If you could ping me when that's the case, please do so! :) |
igniter/bootstrap_repos.py
Outdated
# if the item is directory with major.minor version, dive deeper | ||
try: | ||
ver_dir = item.name.split(".")[ | ||
0] == installed_version.major and \ | ||
item.name.split(".")[ | ||
1] == installed_version.minor # noqa: E051 | ||
if item.is_dir() and ver_dir: | ||
_versions = OpenPypeVersion.get_versions_from_directory( | ||
item) | ||
if _versions: | ||
openpype_versions.append(_versions) | ||
except IndexError: | ||
pass | ||
# if file exists, strip extension, in case of dir don't. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't the item.name
match exactly with f"{installed_version.major}.{installed_version.minor}"
?
So wouldn't this just be if item.name == f"{installed_version.major}.{installed_version.minor}"
or alike?
Should be faster too, and more readable. Less string splitting and just a single comparison. major_minor
could even be defined outside of the iterdir()
loop.
major_minor = f"{installed_version.major}.{installed_version.minor}"
if item.is_dir() and item.name == major_minor:
_versions = OpenPypeVersion.get_versions_from_directory(item)
openpype_versions.extend(_versions)
I also expected this should be openpype_versions.extend()
instead of append when looking at the code?
And shouldn't we also "continue" if we've already identified it as a major minor subfolder, so add
continue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is actually wrong as it shouldn't compare those at all and dive deeper if it matches regex ^\d+\.\d+$
Co-authored-by: Jakub Trllo <[email protected]>
We have deployed this in production now. (Since this requires the build/install to be updated too I've actually batch removed the old install and then batch deployed the new to make sure it's using this newer build logic.) So far so good in Deadline. If anything pops up with errors I'll report back. |
Added UI message if installed version is not compatible with zip and is not launched in headless mode. |
It works. |
Bug
There was a bug that affected finding of last OP version. This resulted in invalid behaviour mainly in headless mode, where it tries to reinstall already installed version.