30005 moment factory #633
30005 moment factory #633
Conversation
| } } | ||
| # SG_PATH_FIELD: { "local_path": d["path"], "name": path_display_name } |
There was a problem hiding this comment.
Thanks for taking the time to get this into a PR!
A few things:
- Your bug fix for stopping the infinite loop in Context is probably something we should incorporate. If you can take care of the few polish-related comments I had there we can see about getting it merged.
- For the additional logic around getting a Context object from a Version entity, I'm a bit concerned. Not because your logic isn't sound, but rather because I'm wary of changing the behavior of the public API in a way that's not backwards compatible. I'm wondering if we can get you the behavior you're looking for in a different way?
- For the root path fix work you've done here, you mention it's likely not needed anymore in the PR summary. Is that logic we should pull out of here, or do you think we could take advantage of it to prevent some future problem clients might run into? Let's see what you think and we'll also loop @manneohrstrom in!
| @@ -806,7 +808,9 @@ def _fields_from_shotgun(self, template, entities, validate): | |||
|
|
|||
| # check the context cache | |||
| cache_key = (entity["type"], entity["id"], key.shotgun_field_name) | |||
| if cache_key in self._entity_fields_cache: | |||
| # MOFA - test for actual cache value, can be None here ... | |||
| # if cache_key in self._entity_fields_cache: | |||
There was a problem hiding this comment.
Let's remove the commented code.
| @@ -806,7 +808,9 @@ def _fields_from_shotgun(self, template, entities, validate): | |||
|
|
|||
| # check the context cache | |||
| cache_key = (entity["type"], entity["id"], key.shotgun_field_name) | |||
| if cache_key in self._entity_fields_cache: | |||
| # MOFA - test for actual cache value, can be None here ... | |||
There was a problem hiding this comment.
Mind removing the "MOFA" headers from your comments? I know it's makes it easy to know where you've made your changes, but if we're going to merge it in, we'll want the comments to look the same as elsewhere!
| @@ -878,6 +882,8 @@ def _fields_from_entity_paths(self, template): | |||
| # are matching the template that is passed in. In that case, try to | |||
| # extract the fields values. | |||
| for cur_path in path_cache_locations: | |||
| # MOFA - bugfix endless loop | |||
There was a problem hiding this comment.
Mind giving an explanation here as to why we can get into a situation where there's an infinite loop? I gather from the comment later in this method that it's tied to some possibly-buggy behavior in os.path.dirname when paired with mounted folders (on Windows?), but a more detailed explanation of what you're seeing will help us understand why the logic is setup the way that it is in the future.
| @@ -1132,6 +1146,23 @@ def _from_entity_type_and_id(tk, entity, source_entity=None): | |||
| # base the context on the project that the published is linked with | |||
| return _from_entity_type_and_id(tk, sg_entity["project"], sg_entity) | |||
|
|
|||
| elif entity_type in ["Version"]: | |||
There was a problem hiding this comment.
It concerns me that we're adding another special case here. I had to tinker with all of this logic back when I was overhauling browser integration for our "zero config" release last year, and I ended up causing some problems that we caught in QA.
I'm going to loop @manneohrstrom here to see if he can chime in. Could we possibly get the behavior we want from "jump to shotgun" in that logic, rather than fundamentally changing what's returned when getting a Context from a Version entity?
There was a problem hiding this comment.
Hey guys! Awesome stuff. I think it makes sense to add version, it has come up a couple of times - wondering if we can just add it to the elif entity_type in ["PublishedFile", "TankPublishedFile"]: part of the logic though? The only thing that is different is the fact that in one place it's task and in the other it's sg_task.
There was a problem hiding this comment.
One more thing - i just realised that we actually have a (still in-progress) PR that is exactly this: check out #633 - i think if we could pull in the relevant unit tests from there into this PR (and then we can close that out), that would be awesome!
| @@ -44,6 +44,11 @@ | |||
|
|
|||
| log = LogManager.get_logger(__name__) | |||
|
|
|||
| # MOFA - imports | |||
There was a problem hiding this comment.
No need for the comment here.
|
|
||
| if path_cache: | ||
| # MOFA - added third value (storage) | ||
| # storage_name, path_cache = _calc_path_cache(tk, path) |
There was a problem hiding this comment.
Let's remove commented code.
There was a problem hiding this comment.
(and elsewhere in this module...)
| # MOFA - test storage value since it's more relevant to check | ||
| # that we were actually able to retrieve a proper LocalStorage entity | ||
| # if path_cache: | ||
| if storage is not None: |
There was a problem hiding this comment.
We used to get a log.warning when storage is None. Is that still the case? If it's not, then let's make this an if storage with the warning, then an else with the logic to handle the storage.
| @@ -724,20 +737,65 @@ def _calc_path_cache(tk, path): | |||
| # append the project name to the root path | |||
| proj_path = "%s/%s" % (norm_root_path, project_disk_name) | |||
|
|
|||
| if norm_path.lower().startswith(proj_path.lower()): | |||
| # MOFA - check proj_path/ to avoid confusion with overlapping project names | |||
| # ie: //server/2017/project vs //server/2017_project | |||
There was a problem hiding this comment.
Good explanation here of why you're jumping through the hoops you are to detect a match. 👍
| field_name = PLATFORM_KEYS.get(sys.platform) | ||
|
|
||
| if field_name is None: | ||
| raise RuntimeError("Unsupported platform {0}".format(sys.platform)) |
There was a problem hiding this comment.
- Let's raise TankError here if we can.
- Also, just for consistency, use
"Unsupported platform %s" % sys.platforminstead of format. As much as I love str.format, we should stick with consistent syntax.
| [[field_name, "is", root_path]], | ||
| ["code"]) | ||
|
|
||
| if not local_storage: |
There was a problem hiding this comment.
Some debug logging here would be nice.
There was a problem hiding this comment.
Also, maybe a detailed comment explaining the search order and situations where one would find the storage and the other wouldn't?
| if cache_key in self._entity_fields_cache: | ||
| # MOFA - test for actual cache value, can be None here ... | ||
| # if cache_key in self._entity_fields_cache: | ||
| if cache_key in self._entity_fields_cache and self._entity_fields_cache[cache_key] is not None: |
There was a problem hiding this comment.
I think this can be shorted down to if self._entity_fields_cache.get(cache_key): ?
It would be great with a comment explaining the case when we are expecting null values inside the _entity_fields_cache dict.
| @@ -892,6 +898,13 @@ def _fields_from_entity_paths(self, template): | |||
| break | |||
| else: | |||
| cur_path = os.path.dirname(cur_path) | |||
| # MOFA - mounted folders can return the same path here | |||
| # maybe not necessary for newer versions of python | |||
| # os.path.dirname is definitely buggy here ... | |||
There was a problem hiding this comment.
Is this something like os.path.dirname("C:\") == "C:\"? If you know of an example, would be great to add it here.
|
Hi! Are those changes still required? We haven't heard from your since our code review and you haven't pushed any updates. |
@pscadding
@jfboismenu
Projects roots fix related to the support request #85520, and to the issue #30005 as far as I could see.
Also including two fixes in contexts (endless loop and keys detection).
Changes are tagged with a # MOFA comment, sorry about that ...
I tried to rewrite my fixRoot function a more shotgunic way to avoid dependencies on my pipeline. this function should not be necessary anymore, but since I still have a bunch of "broken" publishes and filesystem locations on shotgun and cache side, I need to keep it as long as affected projects are active.
HTH !