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

flow: x-schema-name annotation for 2nd-to-last resource path #1572

Merged
merged 11 commits into from
Sep 30, 2024

Conversation

mdibaiee
Copy link
Member

@mdibaiee mdibaiee commented Aug 22, 2024

Description:

  • Sets the base for supporting threading through of the schema name from the second-to-last part of collection name
  • Added tests for both cases (stub config) and sourceCapture

Workflow steps:

Documentation links affected:

(list any documentation links that you created, or existing ones that you've identified as needing updates, along with a brief description)

Notes for reviewers:

(anything that might help someone review this PR)


This change is Reviewable

@mdibaiee mdibaiee force-pushed the mahdi/x-task-name branch 3 times, most recently from a6725df to b4b9bd8 Compare August 27, 2024 12:40
@mdibaiee mdibaiee changed the title flow: x-task-name annotation for 2nd-to-last resource path flow: x-schema-name annotation for 2nd-to-last resource path Aug 27, 2024
@mdibaiee mdibaiee marked this pull request as ready for review August 27, 2024 12:59
Copy link
Member

@psFried psFried left a comment

Choose a reason for hiding this comment

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

I think this is looking directionally right. I left some inline comments, and just wanted to add a general request to rename any collection_name or task_name variables that aren't valid catalog names, since I didn't leave separate comments on each one.

crates/agent/src/integration_tests/harness.rs Outdated Show resolved Hide resolved
crates/agent/src/resource_configs.rs Outdated Show resolved Hide resolved
crates/agent/src/resource_configs.rs Outdated Show resolved Hide resolved
crates/flowctl/src/generate/mod.rs Show resolved Hide resolved
@mdibaiee mdibaiee force-pushed the mahdi/x-task-name branch 4 times, most recently from c00ffa6 to ca6988e Compare September 5, 2024 13:21
@mdibaiee mdibaiee added the change:planned This is a planned change label Sep 9, 2024
Copy link
Member

@psFried psFried left a comment

Choose a reason for hiding this comment

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

Left a few more comments, but I think this is getting pretty close.

crates/agent/src/controllers/materialization.rs Outdated Show resolved Hide resolved
crates/agent/src/resource_configs.rs Outdated Show resolved Hide resolved
crates/models/src/source_capture.rs Show resolved Hide resolved
crates/models/src/source_capture.rs Outdated Show resolved Hide resolved
crates/models/src/source_capture.rs Outdated Show resolved Hide resolved
Copy link
Member

@psFried psFried left a comment

Choose a reason for hiding this comment

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

The remaining issue that I'm seeing is just how to handle the possibility of mismatch between the target_schema and delta_updates fields on the materialization spec, and the resource spec schema of the materialization connector. To me, it seems worth validating that the connector actually has the necessary annotations on the resource spec schema. But it might be worth discussing as others may disagree.

crates/agent/src/controllers/materialization.rs Outdated Show resolved Hide resolved
crates/agent/src/resource_configs.rs Outdated Show resolved Hide resolved
crates/agent/src/resource_configs.rs Outdated Show resolved Hide resolved
@mdibaiee mdibaiee force-pushed the mahdi/x-task-name branch 2 times, most recently from 7d2e5c7 to 45643e3 Compare September 25, 2024 18:53
Copy link
Member

@psFried psFried left a comment

Choose a reason for hiding this comment

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

Left a few more comments/questions

crates/agent/src/integration_tests/harness.rs Show resolved Hide resolved
crates/agent/src/publications/specs.rs Outdated Show resolved Hide resolved
crates/agent/src/publications/specs.rs Outdated Show resolved Hide resolved
crates/agent/src/publications/specs.rs Outdated Show resolved Hide resolved
crates/agent/src/publications/specs.rs Outdated Show resolved Hide resolved
Copy link
Member

@psFried psFried left a comment

Choose a reason for hiding this comment

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

LGTM

@mdibaiee mdibaiee merged commit b84c137 into master Sep 30, 2024
5 checks passed
@mdibaiee mdibaiee deleted the mahdi/x-task-name branch September 30, 2024 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change:planned This is a planned change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants