SG-43421 Add Flow AM desktop publish hooks (Phase 1)#220
Conversation
Move Flow AM publish hooks from tk-config-flowam POC into upstream tk-desktop as a lift-and-shift with no logic changes. Added hooks/flowam/: - collector_desktop.py: Desktop-specific collector extending base collector - publish_to_flow_desktop.py: Desktop-specific publish to Flow AM hook
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #220 +/- ##
==========================================
- Coverage 24.81% 24.71% -0.11%
==========================================
Files 68 68
Lines 3970 3970
==========================================
- Hits 985 981 -4
- Misses 2985 2989 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Restructure the Flow AM publisher hooks to live under hooks/tk-multi-publish2/flowam/ instead of hooks/flowam/. This matches the layout used by other Toolkit engines (tk-nuke, tk-maya), which group their app-specific hooks by consumer app first, then by variant. Also add class-level docstrings to both hooks documenting their dependency on the tk-multi-publish2 base hooks and the expected hook chain string in the config. Changes: - hooks/flowam/collector.py -> hooks/tk-multi-publish2/flowam/collector.py - hooks/flowam/publish_to_flow.py -> hooks/tk-multi-publish2/flowam/publish_to_flow.py
carlos-villavicencio-adsk
left a comment
carlos-villavicencio-adsk
left a comment
There was a problem hiding this comment.
Looks good to me. It's only about code move, so I didn't spend much time reviewing the code.
julien-lang
left a comment
julien-lang
left a comment
There was a problem hiding this comment.
Please add typing info for the new methods
…ype annotation for newly added methods.
Rewrite DesktopFlowPublishPlugin to subclass publish_file.py directly,
eliminating the dependency on FlowPublishPlugin from tk-multi-publish2.
Previously the hook chain included a shared base from tk-multi-publish2:
{self}/publish_file.py:{self}/flowam/publish_to_flow.py:{engine}/tk-multi-publish2/flowam/publish_to_flow.py
Now the desktop plugin is fully self-contained:
{self}/publish_file.py:{engine}/tk-multi-publish2/flowam/publish_to_flow.py
All shared logic (properties, accept, validate, publish, finalize,
get_publish_user) is inlined. _get_flow_args is inlined into _publish_to_flow.
Also add icons/flow.png so self.disk_location resolves the icon correctly.
There was a problem hiding this comment.
Why do we have to add this icon here while we already added it in the parent hook in shotgunsoftware/tk-multi-publish2#217 ?
There was a problem hiding this comment.
If this tk-desktop hook is directly inherit from tk-multi-publish2 hook, then it should use the parent's icon insted. Good call.
| Publishes the file to Flow Production Tracking and Asset Manager. A <b>Publish</b> entry | ||
| will be created in Flow Production Tracking which will include a reference | ||
| to the file's current path on disk. Other users will be able to access the | ||
| published file via the <b><a href='%s'>Loader</a></b> so long as they have |
There was a problem hiding this comment.
Where is %s filled?
HTML attribute should always use double quotes.
| specific revision validation - no super() call needed. | ||
| """ | ||
| # FlowAM framework import | ||
| # TODO: We have an issue on FPTR desktop where the `adsk` cannot be found |
There was a problem hiding this comment.
This was an old warning that should now be fixed with the latest changes we're doing at core level.
| project_valid, project_err = am.validate_project(self.sg_flow_am_id) | ||
| if not project_valid: | ||
| self.logger.error( | ||
| f"No Flow project associated with current SG project: {project_err}" |
There was a problem hiding this comment.
I thought we were going to pass that via the Toolkit context?
There was a problem hiding this comment.
This check can likely be removed. It really just validates whether the flow project attached to the current sg project matches the one that was set for the "flow session". It's a bit redundant.
| pub_info = self.flow_module.asset_management.create_generic_workfile( | ||
| create_inputs, | ||
| ) | ||
| return pub_info |
There was a problem hiding this comment.
Useless variable. Just return the instance.
There was a problem hiding this comment.
I would say it is nice to have the variable only to give the code reader a better idea of what that return value is. But of course, functionality wise it doesn't need to be there.
|
Closing this PR due to the hooks will be place in tk-multi-publish2 instead. |
4 participants
Summary
Moves Flow AM desktop publish hooks from
tk-config-flowamPOC into upstreamtk-desktop(Phase 1).Added
hooks/tk-multi-publish2/flowam/:collector.py- Desktop-specific collector; blocks DCC file extensions and readsTK_FLOWAM_REVISION_ID_*env var set by Loaderpublish_to_flow.py- Self-contained desktop publish plugin (DesktopFlowPublishPlugin)icons/flow.png- Plugin icon resolved viaself.disk_locationDesign:
The original POC used a 3-class hierarchy in the hook chain:
publish_file.py→FlowPublishPlugin(shared base intk-multi-publish2) →DesktopFlowPublishPlugin(desktop leaf intk-desktop). This PR rewritesDesktopFlowPublishPluginto subclasspublish_file.pydirectly, eliminating the dependency on the shared base class fromtk-multi-publish2.The DCC counterpart lives in
tk-multi-publish2asDccFlowPublishPlugin, also self-contained.tk-desktopandtk-multi-publish2can evolve independently across separate release cycles.The hooks are only active with a custom config (
tk-config-flowam), which acts as a feature flag for the Flow AM workflow. Users on any other configuration are unaffected.Config requirements
env/includes/desktop/project.yml:Test plan