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

[Enterprise Search] Add preferences to connectors #150165

Conversation

navarone-feekery
Copy link
Contributor

Summary

Add support for new field preferences in .elastic-connectors.

Related PRs:

Checklist

@navarone-feekery navarone-feekery changed the title Add preferences to connectors [Enterprise Search] Add preferences to connectors Feb 2, 2023
@navarone-feekery navarone-feekery added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.7.0 labels Feb 2, 2023
@navarone-feekery navarone-feekery marked this pull request as ready for review February 2, 2023 13:10
@navarone-feekery navarone-feekery requested a review from a team February 2, 2023 13:10
@sphilipse
Copy link
Member

Ah, I don't think we need the 'label'/'value' construction for these, I don't think we'll pull the name or description for these preferences from the document in any case. We can just give these straight values.

@navarone-feekery
Copy link
Contributor Author

@sphilipse I've changed it to a generic key:value pair, thanks for the feedback!

@sphilipse
Copy link
Member

I'd consider making the contents of preferences explicitly mapped so that we can potentially search over them, like "get me all connectors that have this setting".

@navarone-feekery
Copy link
Contributor Author

@sphilipse I've added extract_full_html as a boolean to the mappings

@@ -27,6 +27,8 @@ export interface CustomScheduling {

export type ConnectorCustomScheduling = Record<string, CustomScheduling | null>;

export type ConnectorPreferences = Record<string, unknown>;
Copy link
Member

@sphilipse sphilipse Feb 2, 2023

Choose a reason for hiding this comment

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

Should update this type too :)
Something like:

export interface ConnectorPreferences {
  extract_full_html: boolean | null;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sphillipse Would something like this work? (keeping in mind that there may be more fields added in the future)

export interface Preferences {
  extract_full_html: boolean;
}
export type ConnectorPreferences = Preferences | Record<string, unknown>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't see your edit, will add. Cheers!

Copy link
Member

Choose a reason for hiding this comment

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

If you wanted to do it in an extensible way, you'd do something like:

export interface ConnectorPreferences extends Record<string, unknown> {
  extract_full_html: boolean | null;
}

Your way is almost correct, but means it's either an object with the defined properties, or an object of shape Record<string, unknown>. In the second case, extract_full_html could be anything--so you lose the advantage of typing. Extending it gets rid of that issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this extensible way so I'm going to use it

@navarone-feekery
Copy link
Contributor Author

@elasticmachine merge upstream

@navarone-feekery navarone-feekery enabled auto-merge (squash) February 2, 2023 15:52
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@navarone-feekery navarone-feekery merged commit f5af84f into elastic:main Feb 2, 2023
navarone-feekery added a commit to navarone-feekery/kibana that referenced this pull request Feb 6, 2023
navarone-feekery added a commit that referenced this pull request Feb 6, 2023
…150328)

This reverts commit f5af84f.

After some team discussion it was decided that `preferences` is not
needed. We will use the existing `configuration` field instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:EnterpriseSearch v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants