Skip to content

Conversation

@lucia-vargas-a
Copy link
Contributor

@lucia-vargas-a lucia-vargas-a commented Nov 29, 2025

Enables adding a metadata label to set the table level as one of [gold, silver, bronze].

Related Tickets & Documents

Reviewer, please follow this checklist

@lucia-vargas-a lucia-vargas-a marked this pull request as ready for review November 29, 2025 12:35
@lucia-vargas-a lucia-vargas-a requested a review from a team as a code owner November 29, 2025 12:35
akkomar
akkomar previously requested changes Dec 1, 2025
Copy link
Collaborator

@akkomar akkomar left a comment

Choose a reason for hiding this comment

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

Requesting changes because of existing approval with test failures.

]

if "level" in metadata:
level = metadata["level"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if you follow Sean's suggestion you could convert to AssetLevel here:

Suggested change
level = metadata["level"]
level = AssetLevel(metadata["level"])

This way invalid values fail at parse time and you don't need a separate validator above.

Copy link
Contributor Author

@lucia-vargas-a lucia-vargas-a Dec 3, 2025

Choose a reason for hiding this comment

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

Very tidy, though I'd personally prefer the detailed debugging messages in the validator, in case of CI failures.

@akkomar akkomar dismissed their stale review December 3, 2025 14:49

Accidental block.

return True


def validate_asset_level(query_dir, metadata):
Copy link
Contributor

Choose a reason for hiding this comment

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

how about missing owners?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion, it makes sense to require owners, at the minimum for level Gold.
Could you add it to Requirements for each level for which levels you think it makes sense?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants