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

Relocate advanced materialization settings in workflows #1417

Conversation

kiahna-tucker
Copy link
Member

@kiahna-tucker kiahna-tucker commented Jan 14, 2025

Issues

The issues directly below are completely resolved by this PR:
#1416

Changes

1416

The following features are included in this PR:

  • Add an Advanced Options accordion section to the Source Collections form section to hide the specification-level onIncompatibleSchemaChange field.

  • Add an Advanced Options accordion section at the bottom of the Config tab in the binding selector to hide the binding-level onIncompatibleSchemaChange and time travel fields.

  • Update the Source Collections section description of the materialization edit workflow to the following: The collections bound to your materialization. Update configuration under the Endpoint Config tab. This content change request is made in the linked issue.

Tests

Manually tested

Approaches to testing are as follows:

  • Validate that the capture workflows remain unchanged.

  • Validate that the onIncompatibleSchemaChange and time travel functionality remain unchanged.

  • Validate that the specification-level onIncompatibleSchemaChange field appears in the Advanced Options accordion section located within the Source Collections form section.

  • Validate that the binding-level onIncompatibleSchemaChange field appears in the Advanced Options accordion section located at the bottom of the Config tab in the binding selector.

Automated tests

N/A

Playwright tests ran locally

  • Admin
  • Captures
  • Collections
  • HomePage
  • Login
  • Materialization

Screenshots

Materialization Create Workflow

Specification-level Advanced Options | Closed

pr_screenshot-1417-hide_advanced_mat_settings-create-spec-closed

Specification-level Advanced Options | Open

pr_screenshot-1417-hide_advanced_mat_settings-create-spec-open

Materialization Edit Workflow

Source Collections section description in materialization edit workflow

pr_screenshot-1417-hide_advanced_mat_settings-source_collections_desc-edit

Specification-level Advanced Options | Closed

pr_screenshot-1417-hide_advanced_mat_settings-spec-closed

Specification-level Advanced Options | Open

pr_screenshot-1417-hide_advanced_mat_settings-spec-open

Binding-level Advanced Options | Closed

pr_screenshot-1417-hide_advanced_mat_settings-binding-closed

Binding-level Advanced Options | Open

pr_screenshot-1417-hide_advanced_mat_settings-binding-open

@kiahna-tucker kiahna-tucker added the change:planned This is a planned change label Jan 14, 2025
@kiahna-tucker kiahna-tucker marked this pull request as ready for review January 15, 2025 19:57
@kiahna-tucker kiahna-tucker requested a review from a team as a code owner January 15, 2025 19:57
Copy link
Member

@travjenkins travjenkins left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines +20 to +32
const fullSourceErrorExists = useBindingStore((state) => {
const fullSourceErrors = state.fullSourceConfigs[bindingUUID]?.errors;

if (!fullSourceErrors) {
return false;
}

return fullSourceErrors.length > 0;
});

const onIncompatibleSchemaChangeErrorExists = useBindingStore(
(state) => state.onIncompatibleSchemaChangeErrorExists.binding
);
Copy link
Member

Choose a reason for hiding this comment

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

Do not need to change for this PR... but I think it would be safe to put these two into a single shallow call. I think that might help reduce how often these might change.

Comment on lines +934 to +942
setOnIncompatibleSchemaChangeErrorExists: (value, key) => {
set(
produce((state: BindingState) => {
state.onIncompatibleSchemaChangeErrorExists[key] = value;
}),
false,
'On Incompatible Schema Change Error Exists Set'
);
},
Copy link
Member

Choose a reason for hiding this comment

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

100% do not want to handle this in the PR. However, I think we should not have scope be involved with setting this and we should eventually add this to the meta data of the binding.

That way the new fields can also show an error icon in the selector.
image

Copy link
Member

@travjenkins travjenkins left a comment

Choose a reason for hiding this comment

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

lgtm - previous one I thought I was marking another PR 🤦

@travjenkins travjenkins merged commit d18e43f into main Jan 21, 2025
3 checks passed
@travjenkins travjenkins deleted the kiahna-tucker/workflows/obscure-advanced-materialization-settings branch January 21, 2025 16:03
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