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

[core, pipes] escaping slashes in asset keys #25742

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

danielgafni
Copy link
Contributor

@danielgafni danielgafni commented Nov 5, 2024

Summary & Motivation

Added support for asset keys containing forward slashes by implementing escape characters. This allows asset keys to include forward slashes within their components while maintaining the ability to use forward slashes as separators between components. For example, "foo/bar" can now be represented as "foo\/bar".

Slashes are escaped before sending the asset key to Pipes and are de-escaped when handling Pipes messages.

How I Tested These Changes

  • Added unit tests for the new functionality
  • Added tests verifying the current behavior of forward slashes in asset keys
  • Tested reporting asset materializations and checks with asset keys containing forward slashes via Pipes

Changelog

Bugfix: [pipes] Added support for escaped forward slashes in asset keys, allowing asset key components to contain forward slashes when properly escaped with backslashes.

Copy link
Contributor Author

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

Join @danielgafni and the rest of your teammates on Graphite Graphite

called = {}

def _impl(context: PipesContext):
breakpoint()
Copy link

Choose a reason for hiding this comment

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

Please remove the breakpoint() statement before merging to avoid unintended debugging interruptions in production code.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

@danielgafni danielgafni changed the title [pipes] fixes for asset keys of length > 1 [core, pipes] escaping slashes in asset keys Nov 5, 2024
@danielgafni danielgafni force-pushed the 11-04-_pipes_asset_keys_escaping branch from 186790c to 9c275fb Compare November 5, 2024 11:47
@danielgafni danielgafni force-pushed the 11-04-_pipes_asset_keys_escaping branch from 9c275fb to e2f824b Compare November 5, 2024 12:45
@danielgafni danielgafni marked this pull request as ready for review November 5, 2024 23:03
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.

1 participant