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

Initial wiring up for capture interval #1230

Merged
merged 13 commits into from
Aug 26, 2024

Conversation

travjenkins
Copy link
Member

@travjenkins travjenkins commented Aug 23, 2024

Issues

Starting #1194

Changes

1194

  • Getting the default_capture_interval stubbed in (but commented out)
  • Cleaning up the connector tag query a little bit
  • Adding a guard to ensure we have the connector ID we need

Misc

  • Making a place to keep validation related stuff cause that is hard to find sometimes
  • Added guard for catalog name while loading details
  • Add eventing for when we show Entity not found page
  • Fixing issue with time travel regex being invalid

Tests

Manually tested

  • scenarios you manually tested

Automated tests

  • unit testing covered

Playwright tests ran locally

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

Screenshots

If applicable - please include some screenshots of the new UI

}
RediscoverButton={
<RediscoverButton
<WorkflowHydrator>
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the actual change... grouped all hydrators into a shared spot

Comment on lines +87 to +90
const canBeEmpty = useMemo(
() => configCanBeEmpty(connectorTag?.endpoint_spec_schema),
[connectorTag?.endpoint_spec_schema]
);
Copy link
Member Author

Choose a reason for hiding this comment

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

Since we are using this function in the store want to make sure they use the same logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

The new guard to ensure we have a connector selected on .../create/new.

@@ -193,5 +193,5 @@ export const getDereffedSchema = async (val: any) => {
};

export const configCanBeEmpty = (schema: any) => {
return Boolean(!schema.properties || isEmpty(schema.properties));
return Boolean(!schema?.properties || isEmpty(schema?.properties));
Copy link
Member Author

Choose a reason for hiding this comment

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

wanted to make this code just a bit more safe.

// TODO (Validators) we need to build out validators for specific types of data
details.data.connectorImage.connectorId &&
details.data.connectorImage.connectorId.length === 23 &&
MAC_ADDR_RE.test(details.data.connectorImage.connectorId) &&
Copy link
Member Author

Choose a reason for hiding this comment

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

We now have a regex for this so using that

Copy link
Member Author

Choose a reason for hiding this comment

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

Validator for details page to ensure the catalog name in the URL is valid.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the new guard to the two ...CreateNew files

Comment on lines +17 to +21
useMount(() => {
logRocketEvent(CustomEvents.ENTITY_NOT_FOUND, {
catalogName,
});
});
Copy link
Member Author

Choose a reason for hiding this comment

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

Added the event as we are now validating catalog names on details and want to be safe... but also makes sense to track if this component is shown.

Copy link
Member Author

Choose a reason for hiding this comment

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

Starting to move some validation stuff over to validation so that can all be in one spot.

// Based on the patterns connectors use for date time
// eslint-disable-next-line no-useless-escape
export const DATE_TIME_PATTERN = `[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z`;
export const DATE_TIME_RE = new RegExp(`^(${DATE_TIME_PATTERN})$`);
Copy link
Member Author

Choose a reason for hiding this comment

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

This was copied from old location. But ended up being invalid because it had an opening ( but not a closing one. So added the new closing one.

@travjenkins travjenkins marked this pull request as ready for review August 23, 2024 20:39
@travjenkins travjenkins requested a review from a team as a code owner August 23, 2024 20:39
Copy link
Member

@kiahna-tucker kiahna-tucker left a comment

Choose a reason for hiding this comment

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

Approved without manual testing.

src/validation/index.ts Outdated Show resolved Hide resolved
src/lang/en-US.ts Outdated Show resolved Hide resolved
'workflows.interval.header': `Polling Interval`,
'workflows.interval.message': `Allows controlling how often the Capture will check for new data. Intervals are relative to the start of an invocation and not its completion. For example, if the interval is five minutes, and an invocation of the capture finishes after two minutes, then the next invocation will be started after three additional minutes.`,
'workflows.interval.input.label': `Interval`,
'workflows.interval.input.seconds': `seconds`,
Copy link
Member

Choose a reason for hiding this comment

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

We use time-based units often. Would it make sense to move these to CommonMessages?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I wasn't sure... I was feeling like maybe we make a Units grouping makes the most sense cause it feels like Common is starting to get too full.

@travjenkins travjenkins merged commit 11a8477 into main Aug 26, 2024
3 checks passed
@travjenkins travjenkins deleted the travjenkins/feature/capture-interval branch August 26, 2024 13:57
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.

2 participants