search

ynput / ayon-kitsu

Official AYON<->Kitsu intetgration (WIP)
Apache License 2.0
7 stars 4 forks source link

Additional tests and fixes #35

Open scottmcdonnell opened 5 months ago

scottmcdonnell commented 5 months ago

I had a few issues with the 1.1.0 sync.

Lastly some of new bits were using the Ayon api via httpx to save users/projects etc but this is mixed with the existing server/kitsu/utils.py which uses Postgres for gets and Entity models to save and update. This I found really confusing, it should be all one or the other. It didn't make much sense to me for Ayon (in an ayon api endpoint) to be calling itself through the api when the models are available and the postgres connection is already initialised, it seems an unnecessary overhead, so I converted the api calls to model calls in utils.py to make it consistent.

I saw afterwards that this was discussed on discord a bit, and the preference was API, but I think it should be consistent throughout utils.py - so not sure what the team thinks on this one!

Screenshot 2024-02-25 at 15 00 48

scottmcdonnell commented 4 months ago

added support for LauncherAction https://github.com/ynput/ayon-kitsu/issues/36

EmberLightVFX commented 4 months ago

Would it be possible to change the arrow-icon to the Kitsu icon for the "Show in Kitsu" button? You can get their icon here: https://www.cg-wire.com/_nuxt/logo-kitsu.de716c4b.png (might want to make it smaller)

iLLiCiTiT commented 3 months ago

Hello, could you update the changes? Could you also split the PR into more PRs? There is too much changes for review (smaller PRs -> faster review).

scottmcdonnell commented 2 months ago

Hello, could you update the changes? Could you also split the PR into more PRs? There is too much changes for review (smaller PRs -> faster review).

Thanks for taking a look. I can give it a go breaking it down into different PRs. Before I go ahead can I ask you to review one big structural change in this PR to see if I should proceed down this path?

on the server side there is a mix between using EntityModels and api calls in the current code. This PR replaces any api with Entity in order to make it consistent. From above:

Lastly some of new bits were using the Ayon api via httpx to save users/projects etc but this is mixed with the existing server/kitsu/utils.py which uses Postgres for gets and Entity models to save and update. This I found really confusing, it should be all one or the other. It didn't make much sense to me for Ayon (in an ayon api endpoint) to be calling itself through the api when the models are available and the postgres connection is already initialised, it seems an unnecessary overhead, so I converted the api calls to model calls in utils.py to make it consistent.

For example: https://github.com/ynput/ayon-kitsu/pull/35/commits/7559d8b5c3365c9e5380b75240e3558160f293c8

iLLiCiTiT commented 2 months ago

Before I go ahead can I ask you to review one big structural change in this PR to see if I should proceed down this path?

The issue is that different changes should be reviewed by different people, that's why I asked for split. It is hard to tell which change is related to what.

There are some small bugfixes and big feature changes next to it (multiple features). The features usually needs discussion or may not be merged because of different opinion, but the bugfixes can be merged right away.

scottmcdonnell commented 2 months ago

Before I go ahead can I ask you to review one big structural change in this PR to see if I should proceed down this path?

The issue is that different changes should be reviewed by different people, that's why I asked for split. It is hard to tell which change is related to what.

There are some small bugfixes and big feature changes next to it (multiple features). The features usually needs discussion or may not be merged because of different opinion, but the bugfixes can be merged right away.

Fair enough, I get that. Would it be possible to get feedback on the replacement of API for Entities please? https://github.com/ynput/ayon-kitsu/commit/7559d8b5c3365c9e5380b75240e3558160f293c8

If that is given the thumbs up or down I will know how to proceed with the PRs.

iLLiCiTiT commented 2 months ago

Fair enough, I get that. Would it be possible to get feedback on the replacement of API for Entities please? https://github.com/ynput/ayon-kitsu/commit/7559d8b5c3365c9e5380b75240e3558160f293c8

@martastain

Note that it is hard to tell, because for example I don't know which exact changes are related to it.

m-u-r-p-h-y commented 2 months ago

First of all thank you @scottmcdonnell for your contribution! To push it through, we really need to split this massive PR to smaller pieces as it is not approvable in the current state.

each bulleted item in your description should be a separate PR honestly.

scottmcdonnell commented 2 months ago

First of all thank you @scottmcdonnell for your contribution! To push it through, we really need to split this massive PR to smaller pieces as it is not approvable in the current state.

each bulleted item in your description should be a separate PR honestly.

No problem, I will break into smaller PRs. I am on a few big projects at the moment but will come back to this as soon as I can.

BigRoy commented 1 month ago

Would be great to get some 🔥 progress into this one. Someone else just hit what seems like something that this PR may solve? See on Ynput discord here.

scottmcdonnell commented 1 month ago

Would be great to get some 🔥 progress into this one. Someone else just hit what seems like something that this PR may solve? See on Ynput discord here.

Sorry was very snowed under. PRs started. Can I make PRs dependant on each other? The testing requires fixtures and set up that is common to each PR and some changes have larger dependencies.

MustafaJafar commented 3 weeks ago

Hello, I think this PR is big, would be better to break it down into smaller PRs ? e.g. could we have separate PR for the launcher action ?