Skip to content

Conversation

@alangenfeld
Copy link
Member

@alangenfeld alangenfeld commented Apr 22, 2025

Currently we just have AssetAttributes which we use to power both Definitions post processing as well as pre-definitions AssetSpec customization.

Unfortunately, these two context have different constraints. We do not support changing the key of an asset spec inside of an already formed AssetsDefinition as its difficult to ensure it could safely handle that transformation in all cases.

This change separates the two cases, and adds key_prefix for the spec only update condition.

How I Tested These Changes

added tests

Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@alangenfeld alangenfeld force-pushed the al/04-22-_assetpostprocessor_key_and_key_prefix_support branch from aa2d3a6 to c605c64 Compare April 22, 2025 18:44
@alangenfeld alangenfeld force-pushed the al/04-22-_assetpostprocessor_key_and_key_prefix_support branch from c605c64 to a37e90b Compare April 22, 2025 18:47
Copy link
Contributor

@OwenKephart OwenKephart left a comment

Choose a reason for hiding this comment

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

(as discussed offline) given that this doesn't work with AssetsDefinitions (just asset specs), and I would expect AssetsDefinitions to be more common, I feel like we should simply not support this pattern, even though it's a bit of a bummer.

generally, I think asset keys should be used purely as unique identifiers that are very domain-specific, and components themselves are often going to be the best place to encode that domain-specific information. adding prefixes at hierarchy levels feels a bit like an anti-pattern if it's used for organization (as you can use tags or groups for that purpose)

@alangenfeld alangenfeld force-pushed the al/04-22-_assetpostprocessor_key_and_key_prefix_support branch from a37e90b to a4872ce Compare April 22, 2025 19:57
@alangenfeld alangenfeld changed the title [AssetPostProcessor] key and key_prefix support [resolved] split asset defs update and asset spec update handling Apr 22, 2025
@alangenfeld alangenfeld requested a review from OwenKephart April 22, 2025 20:00
@alangenfeld alangenfeld force-pushed the al/04-22-_assetpostprocessor_key_and_key_prefix_support branch 3 times, most recently from b53355f to e0d79a8 Compare April 22, 2025 20:30
Copy link
Contributor

@OwenKephart OwenKephart left a comment

Choose a reason for hiding this comment

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

nice, much prefer this

@alangenfeld alangenfeld force-pushed the al/04-22-_assetpostprocessor_key_and_key_prefix_support branch from e0d79a8 to d06b8a4 Compare April 22, 2025 21:59
Copy link
Member Author

alangenfeld commented Apr 23, 2025

Merge activity

  • Apr 23, 11:40 AM CDT: A user started a stack merge that includes this pull request via Graphite.
  • Apr 23, 11:41 AM CDT: A user merged this pull request with Graphite.

@alangenfeld alangenfeld merged commit 468ba9e into master Apr 23, 2025
6 checks passed
@alangenfeld alangenfeld deleted the al/04-22-_assetpostprocessor_key_and_key_prefix_support branch April 23, 2025 16:41
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.

3 participants