getavalon / core

The safe post-production pipeline - https://getavalon.github.io/2.0
MIT License
214 stars 48 forks source link

Fix#246 : Store and lock the "family" on a "subset" #443

Closed davidlatwe closed 4 years ago

davidlatwe commented 4 years ago

This PR aim to resolve #246, the implementation I am proposing is to involve new schema for subset and version, so that we could have a standard to maintain backwards compatibility.

What's changed?

davidlatwe commented 4 years ago

What I have tested:

BigRoy commented 4 years ago

I've been giving this a roll and backwards compatibility seems smooth, great work! 🚀 I didn't notice it had changed from an Artist's perspective.

However, I did have a question. How did you end up updating your integrator?

Be aware when updating your integrator: I moved the storing of the families data to the subset as opposed to the version. However, of course an existing subset would still have the previous schema and wasn't "created" for this publish. As such, suddenly I have a subset without families (old one) and the new version also without families. The result was that the subset wouldn't show in the Loader.

Should have thought of that of course. :) Just wondering how you adapted for this situation @davidlatwe.

davidlatwe commented 4 years ago

Ah, have you updated the version and subset schema name in your integrator plugin ? ~Here's my change for this PR in my config, :point_right: :door: (plugins/global/publish/integrate_avalon_subset.py)~ Edit: :point_up: this isn't correct, see the comment below

davidlatwe commented 4 years ago

Oh, wait, I might misunderstand your case, let me take a look

BigRoy commented 4 years ago

Ah, I see. You have it as an if/else deciding between putting families in the data of subset/version here based on subset schema version. Simple enough.

I ended up actually updating my subset version:

# example psuedocode
if subset and subset["schema"] == "avalon-core:subset-2.0":
    self.log.info("Updating subset {name} "
                  "'{schema}' to schema "
                  "'avalon-core:subset-3.0'".format(**subset))
    subset_original = copy.deepcopy(subset)
    subset["schema"] = "avalon-core:subset-3.0"
    subset["data"]["families"] = _get_families(instance)
    schema.validate(subset)
    result = io.replace_one(subset_original, subset)
    assert result.modified_count == 1, "Subset schema update failed."

I guess your way should have been the way to go though, to leave whatever was as close to the originals as possible.

~Anyway, it seems I am unable to find the new version 3 subset document in the Loader.~ Nevermind, false alarm. Somehow my Python session was using an older loader, restarted the host and all was fine.

👍 Smooth sailing from here on out.

davidlatwe commented 4 years ago

Okay, I get the picture now :P

Should have thought of that of course. :)

I actually didn't ! Big thanks for the heads up ! Yeah, the old subset required to be updated so it could work with new version that use new schema.

But I think the best and simple option for anyone prepare to adopt this PR is :

So that you won't need to worry breaking anything already existed. :)

davidlatwe commented 4 years ago

To be complete, here's what I have for my config to adopt this PR. :point_right: :door: