Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chore: Add 'parents' to folder template data #778

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

iLLiCiTiT
Copy link
Member

@iLLiCiTiT iLLiCiTiT commented Jul 15, 2024

Changelog Description

Add "parents" subkey to folder template data.

Additional info

This does allow to use indexing of parent names in templates. I'm not sure if this is correct approach, but it is the best I could think of. This change does not consider that someone would like to use e.g. labels instead of names in future, hopefully that won't happen.

There is no safeguard for using the list, so if the parent index is not available it will just crash. That might cause issues in specific logic which is filling template paths and does not count on this.

Testing notes:

  1. You can use e.g. {folder[parents[0]]} in templates.
  2. Test it with an optional template key <{folder[parents[0]}> works from a ROOT folder (does not have any parent folders).

@ynbot
Copy link
Contributor

ynbot commented Jul 15, 2024

@ynbot ynbot added size/XS type: enhancement Improvement of existing functionality or minor addition labels Jul 15, 2024
@iLLiCiTiT iLLiCiTiT added the sponsored This is directly sponsored by a client or community member label Jul 15, 2024
@BigRoy
Copy link
Collaborator

BigRoy commented Jul 15, 2024

This does allow to use indexing of parent names in templates. I'm not sure if this is correct approach, but it is the best I could think of. This change does not consider that someone would like to use e.g. labels instead of names in future, hopefully that won't happen.

There is no safeguard for using the list, so if the parent index is not available it will just crash. That might cause issues in specific logic which is filling template paths and does not count on this.

Hmm - yeah, hmm.. this is bound to give issues. If anyone would use e.g. parents[-1] in a template and they are working in an asset that has no parents the template formatting would already fail. This seems very prone to production issues.

Can we re-iterate why parents is needed as a key and what the admin/user is expected to do with it?

@jakubjezek001
Copy link
Member

asset that has no parents the template formatting would already fail. This seems very prone to production issues.

I wonder if we could use for that reason optional decoration ...}</{folder[parents[-1]]}>/...

@jakubjezek001
Copy link
Member

BTW this seems to be very good case for unittesting.

@@ -407,8 +407,10 @@ def _fill_folder_data(self, instance, project_entity, anatomy_data):
anatomy_data["hierarchy"] = hierarchy

parent_name = project_entity["name"]
parents = []
Copy link
Member

@jakubjezek001 jakubjezek001 Jul 16, 2024

Choose a reason for hiding this comment

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

It seems valid to always have parents filled with at least project, right? Since we are converting parent_name from project name in case there is no parent at all it would be fair to also add it. In this case {folder[parent[-1]]} would always work.

Copy link
Member Author

@iLLiCiTiT iLLiCiTiT Jul 16, 2024

Choose a reason for hiding this comment

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

I really don't think project should be there. But the use-case is that they would like to be able to use {folder[parents][0]} and {folder[parents][1]} not really -1. For -1 you can use {parent}.

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 project name should be there too - the reason why is that you should be explicit enough if you want to have project name there (and you can access it with another key).

So if you have asset folder directly under project, {folder[parents][0]} will not work if I am not mistaken.

Copy link
Member Author

Choose a reason for hiding this comment

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

So if you have asset folder directly under project, {folder[parents][0]} will not work if I am not mistaken.

Yes, it won't.

@BigRoy
Copy link
Collaborator

BigRoy commented Jul 16, 2024

asset that has no parents the template formatting would already fail. This seems very prone to production issues.

I wonder if we could use for that reason optional decoration ...}</{folder[parents[-1]]}>/...

Ah - if that works I must admit that's fine then I suppose.

@BigRoy
Copy link
Collaborator

BigRoy commented Jul 29, 2024

So we should add to the testing notes:

  • Test it with an optional template key <{folder[parents[0]}> and make sure it works from a ROOT folder (no parent folders)?

Copy link
Member

@LiborBatek LiborBatek left a comment

Choose a reason for hiding this comment

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

I have tried to incorporate it into template for render products as this:

Screenshot 2024-07-31 150417

but constantly getting errors when publish job on DL takes place after succesful render job....

see DL publish job log below:

Job_2024-07-31_15-02-09_66aa35d12c5b90b0d2104221.txt

@iLLiCiTiT
Copy link
Member Author

Something I didn't think of is output of used values which right now in StringTemplates always outputs dictionary, that has to be changed to support list.

Another issue might be negative index {folder[parents][-1]} would not work either as python formatting resolves -1 as string instead of number.

@iLLiCiTiT iLLiCiTiT marked this pull request as draft August 14, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS sponsored This is directly sponsored by a client or community member type: enhancement Improvement of existing functionality or minor addition
Projects
Status: Review In Progress
Development

Successfully merging this pull request may close these issues.

6 participants