Skip to content
This repository has been archived by the owner on Sep 20, 2024. It is now read-only.

Module: Kitsu module #2650

Merged
merged 84 commits into from
Jun 7, 2022
Merged

Module: Kitsu module #2650

merged 84 commits into from
Jun 7, 2022

Conversation

Tilix4
Copy link
Collaborator

@Tilix4 Tilix4 commented Feb 3, 2022

Brief description

Kitsu module.

Description

Use Kitsu as project management module.

Documentation (add "type: documentation" label)

Doc has been written, you can read it in PR files.

Testing notes:

  1. Fill Server url in Project Settings.
  2. When launching Tray, Kitsu Credentials window will pop. Either way click on Kitsu Connect button and sign-in.
  3. Run .poetry/bin/poetry run python start.py module kitsu sync-service -l me@domain.ext -p mypass to start sync OP with Kitsu, events listening will start right after sync is achieved.
  4. Publish a new version of an asset and see the magic happening.

You can also try .poetry/bin/poetry run python start.py module kitsu push-to-zou -l me@domain.ext -p mypass, but it cannot handle all use cases and must be considered as a helpful facultative utility.

Listening currently supports most of project/episode/sequence/shot/asset/task create/update/delete events, but not project:delete, episode:delete, sequence:delete because of a bug, which will be resolved by cgwire/zou#427

@antirotor antirotor added the module: Kitsu Kitsu integration label Feb 8, 2022
@BigRoy
Copy link
Collaborator

BigRoy commented Feb 8, 2022

@Tilix4 great to see you are looking into tackling this! 💪

I didn't see any related issue regarding this so I just wanted to point out this ancient! set of tool prototypes I did for gazu (Python client for Kitsu) called qtazu. I am not sure how well that code holds up Today but it could be a good reference for maybe kitsu related code if it is still somewhat valid.

Copy link
Member

@mkolar mkolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome to see work being done on Kitsu front. Just an important note for start, that we are trying to get rid of the default_modules folder. Could you please move the module one level up?

Actually @iLLiCiTiT just told me that for now this is easier for you, so just keep in mind that it will eventually move please.

@Tilix4
Copy link
Collaborator Author

Tilix4 commented Feb 24, 2022

Awesome to see work being done on Kitsu front. Just an important note for start, that we are trying to get rid of the default_modules folder. Could you please move the module one level up?

Actually @iLLiCiTiT just told me that for now this is easier for you, so just keep in mind that it will eventually move please.

Yes I started there because @iLLiCiTiT told me so. I'd be glad to have help to move it one level up :)

@iLLiCiTiT
Copy link
Member

I'd be glad to have help to move it one level up :)

When you move it, you have to add "kitsu" (name of folder where module is) into DEFAULT_OPENPYPE_MODULES variable in openpype/modules/base.py.

@Tilix4
Copy link
Collaborator Author

Tilix4 commented Mar 1, 2022

@iLLiCiTiT @mkolar I moved the module up.

I think the module is ready to be tested for basic usage, some things are not working (project:delete, episode:delete, sequence:delete) and are about to be fixed on Kitsu's side: cgwire/zou#427

I also commited the pyproject.toml with gazu.

I'll now write some documentation but I cannot find any guideline about how to write documentation for OP, did I miss something?

@mkolar
Copy link
Member

mkolar commented Mar 1, 2022

I'll now write some documentation but I cannot find any guideline about how to write documentation for OP, did I miss something?

There aren't docs about the docs :D. But it is simple enough. We use docusaurus and all it's content is inside https://github.com/pypeclub/OpenPype/tree/develop/website

To add new documents you just add .md pages to /docs folder and add the new entry into sidebars.js.

Docusaurus itself has very good documentation https://docusaurus.io/docs (surprise, surprise :D ) if you want to see more details.

Other than that, this page shows some example formatings https://github.com/pypeclub/OpenPype/blob/develop/website/docs/admin_docsexamples.md

And if you want to run it locally to test, you'll need to run yarn install inside of the website folder and then launch the docs with the provided tool https://github.com/pypeclub/OpenPype/blob/develop/tools/run_documentation.ps1

Copy link

@hound hound bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files could not be reviewed due to errors:

./openpype/modules/kitsu/utils/credentials.py:11:49: B008: Do not perform fun...
./openpype/modules/kitsu/utils/credentials.py:11:49: B008: Do not perform function calls in argument defaults.  The call is performed only once at function definition time. All calls to your function will reuse the result of that definition-time function call.  If this is intended, assign the function call to a module-level variable and use that variable as a default value.

@mkolar mkolar added type: documentation type: feature Larger, user affecting changes and completely new things Bump Minor Pull requests that update a dependency file labels Mar 4, 2022
@ClementHector
Copy link
Contributor

ClementHector commented Apr 20, 2022

The data put in asset_data["tasks"][TASK_NAME]["zou"] are removed if you make changes to the task in the project manager interface. Do we have to change the place to put the zoo data of the task or modify the behavior of the project manage?

@Tilix4
Copy link
Collaborator Author

Tilix4 commented Apr 20, 2022

The data put in asset_data["tasks"][TASK_NAME]["zou"] are removed if you make changes to the task in the project manage interface. Do we have to change the place to put the zoo data of the task or modify the behavior of the project manage?

That's a great question, maybe @iLLiCiTiT has an answer?

@mkolar
Copy link
Member

mkolar commented Apr 28, 2022

That's a great question, maybe @iLLiCiTiT has an answer?

@Tilix4 @ClementHector yeah to get around that we'd have to tweak some project manager logic. This two way mapping is actually the reason why we don't currently recommend using ftrack and our project manager at the same time. PM was really added just for micro productions that don't have any third party tool.

@ClementHector
Copy link
Contributor

@mkolar @Tilix4 I worked around the problem in the plugins, the collector will look for the data in another way if it no longer exists in asset_data["tasks"][TASK_NAME]["zou"] . See Tilix4#3 in openpype/modules/kitsu/plugins/publish/collect_kitsu_entities.py

if not publish_status:
self.log.info("Status is not set.")

kitsu_status = gazu.task.get_task_status_by_short_name(publish_status)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intent is meant as "Intent of publishing" not status of task.

Copy link
Collaborator Author

@Tilix4 Tilix4 May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get your remark, if you may explain further, please?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, intent is artist's reason of publishing (e.g. "test" or "wip") but it's used here to check existence of status. Status is just a different thing then intent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean, when intent is not None, we must avoid updating the status in Kitsu, and when it's None, change the status to Waiting for Approval?

Copy link
Collaborator Author

@Tilix4 Tilix4 May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our case, this intent code seems out of topic, must be deleted, and status change must be moved to IntegrateKitsuNote.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made the relevant changes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our case, this intent code seems out of topic, must be deleted, and status change must be moved to IntegrateKitsuNote.

That's correct. Intent is technically a unified comment thing that is requested by certain vendors and studios. For instance when artist publishes first version of a comp it might have status for review but intent might just be tech validation.... later on v03 would still go to for review with intent wip , and at the end v06 might be for review and intent for final... or similar. intents are of course arbitrary in each studio, but most importantly they are static data on a version and don't change with status updates


def process(self, context):
# Check if work version for user
is_work_version = bool(context.data.get("intent", {}).get("value"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when intent is not available then note is not integrated -> and review is not integrated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, actually this is the opposite. If the intent has no value, it's supposed to be a normal publish. If an intent is set (WIP, test), then the Production tracker is not updated but OP is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that intent should affect if note or review is integrated at all. Intent can be "For review", "Final" or anything else, dynamically created by studio.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean, it shouldn't affect note or review by default, it's up to the studio to decide to override or not this behavior?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, intent is just an information from artist that should be reflected somewhere, like a "comment" with predefined messages. Should not be used as definition of publish flow, at least not by default.

NOTE: Intent is OpenPype specific thing which is not in pyblish-base/lite/qml.

Copy link
Collaborator

@BigRoy BigRoy May 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the intent has no value, it's supposed to be a normal publish. If an intent is set (WIP, test), then the Production tracker is not updated but OP is.

If really needed then this sounds more like a Admin Settings tweak like a "filter" for when to integrate to Kitsu. For example:

  • Settings for "Exclude Intents for Integration to Kitsu" could be set to {"WIP", "test"}
  • The integrator would just check against that if intent in exclude_intents: return.

Should not be used as definition of publish flow, at least not by default.

👍 - Is it correct there's nothing native to OpenPype that would allow this in production currently?

Copy link
Collaborator Author

@Tilix4 Tilix4 May 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not be used as definition of publish flow, at least not by default.

Okay, will do it this way.

If really needed then this sounds more like a Admin Settings tweak like a "filter" for when to integrate to Kitsu.

I don't need it currently, with @ClementHector we thought we should do it this way. Then we'll wait for feedback.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Publish comments can be integrated into kitsu/ftrach... with the review, right?

Copy link
Member

@iLLiCiTiT iLLiCiTiT May 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Settings for "Exclude Intents for Integration to Kitsu" could be set to

I would say that this integrator plugin should not look for intent at all, but should have mechanism to skip if "some value" is set (or not set).

Like, what if I don't care about intent at all and just want integrate everything, all the time. With this logic I also have to care about intent even if it should not matter. But I can have a plugin which will "based on intent value" store value into instance/context which tell this plugin to skip integration, or based on comment, or based on separated settings from my addon.

@mkolar mkolar self-requested a review May 31, 2022 15:01
@ClementHector
Copy link
Contributor

@mkolar Hello, Is there something to do to accept this request?

@mkolar
Copy link
Member

mkolar commented Jun 7, 2022

I'd say this is ready to go!

@mkolar
Copy link
Member

mkolar commented Jun 7, 2022

Kitsu meet OpenPype, OpenPype meet Kitsu. Play nice together and don't make much mess......

@mkolar mkolar merged commit 548f2a6 into ynput:develop Jun 7, 2022
@mkolar
Copy link
Member

mkolar commented Jun 7, 2022

@all-contributors please add @Tilix4 for code and documentation

@allcontributors
Copy link
Contributor

@mkolar

I've put up a pull request to add @Tilix4! 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bump Minor Pull requests that update a dependency file module: Kitsu Kitsu integration type: documentation type: feature Larger, user affecting changes and completely new things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants