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.

Sign up for GitHub

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

Jump to bottom

[App Search] New flyout to start a crawl with custom settings #124999

Merged
merged 13 commits into from
Feb 15, 2022
Merged

[App Search] New flyout to start a crawl with custom settings #124999

merged 13 commits into from
Feb 15, 2022

Conversation

byronhulcher
Copy link
Contributor

@byronhulcher byronhulcher commented Feb 8, 2022 •
edited
Loading

Summary

Adds a new flyout to the Crawler that allows a user to set the following fields as a part of a crawl request:

  • domain_allowlist
  • seed_urls (which we refer to as "entry points" in the UX and the JS code)
  • sitemap_urls
  • max_crawl_depth
  • sitemap_discovery_disabled

In addition to allowing the user to select sitemap and entry point urls they've already added to their crawler's config, we give the user two EuiComboBox components to add custom sitemaps and entry points.

Screenshots

Screen.Recording.2022-02-11.at.3.35.45.PM.mov

QA

You'll need to be running the branch from PR https://github.com/elastic/ent-search/pull/6064 locally in order to test this while that PR remains open.

Checklist

@byronhulcher byronhulcher added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v8.2.0 labels Feb 8, 2022
@byronhulcher byronhulcher marked this pull request as ready for review February 11, 2022 20:46
@byronhulcher byronhulcher requested a review from a team as a code owner February 11, 2022 20:46
@byronhulcher byronhulcher requested review from a team, daveyholler and sphilipse February 11, 2022 20:46
@byronhulcher byronhulcher self-assigned this Feb 11, 2022
@byronhulcher byronhulcher force-pushed the customize-crawl-flyout branch from 41a4cde to b2de2bf Compare February 11, 2022 20:50
daveyholler
daveyholler reviewed Feb 11, 2022
View reviewed changes
...ons/app_search/components/crawler/components/crawl_custom_settings_flyout/url_combo_box.scss
@@ -0,0 +1,5 @@
.urlComboBox {
.euiComboBox__inputWrap {
min-height: 60px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Only suggestion here would be to use a rem value instead of the px... Should work out to 3.75rem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I got 60px because the default row height of a row in an EuiComboBox is 29px + 1px = 30px, which I doubled so it defaults to two rows. There's no way to specifically set the default number of rows for EuiComboBox unfortunately. With that in mind should I still convert to rem? Should I leave as-is but write a comment? I'll follow your call

@byronhulcher
Copy link
Contributor Author

@elasticmachine merge upstream

orhantoy
orhantoy reviewed Feb 15, 2022
View reviewed changes
Copy link
Contributor

@orhantoy orhantoy left a comment

Choose a reason for hiding this comment

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

👏 Really excited about this feature, thanks Byron!

...onents/crawler/components/crawl_custom_settings_flyout/crawl_custom_settings_flyout_logic.ts
);

return seedUrls.filter((seedUrl) => {
const { domain } = extractDomainAndEntryPointFromUrl(seedUrl);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if it would be more robust to filter the response from the /domain_configs endpoints based on the domain, instead of trying to match the domain names?

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 need to think about how to do this in the context of kea logic a little more. There's probably a way to accomplish this without having to pass domainConfigs in through the onSelectDomainUrls action (using a selector?) but its not totally clear to me at the moment.

...onents/crawler/components/crawl_custom_settings_flyout/crawl_custom_settings_flyout_logic.ts Outdated
onSelectSitemapUrls(sitemapUrls: string[]): { sitemapUrls: string[] };
showFlyout(): void;
startCustomCrawl(): void;
toggleIncludeRobotsTxt(): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

The naming here is slightly (semantically) incorrect because the toggle is to include/exclude sitemaps from robots.txt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for clarifying this @orhantoy, I addressed this in 8d9ca6f

...ions/app_search/components/crawler/components/crawl_custom_settings_flyout/url_combo_box.tsx
: undefined
}
>
<EuiComboBox
Copy link
Contributor

Choose a reason for hiding this comment

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

This is neat! I initially thought a simple textbox would be more flexible when for instance editing an entry URL but it's pretty cool that we can have validations here via onCreateOption.

@byronhulcher byronhulcher enabled auto-merge (squash) February 15, 2022 21:24
@byronhulcher byronhulcher merged commit b11a829 into elastic:main Feb 15, 2022
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
enterpriseSearch 1355 1367 +12

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
enterpriseSearch 1.3MB 1.3MB +11.5KB

History

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

cc @byronhulcher

@kibanamachine
Copy link
Contributor

💔 Backport failed

The pull request could not be backported due to the following error:
There are no branches to backport to. Aborting.

How to fix

Re-run the backport manually:

node scripts/backport --pr 124999

Questions ?

Please refer to the Backport tool documentation

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 17, 2022
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 124999 or prevent reminders by adding the backport:skip label.

6 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 124999 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 124999 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 124999 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 124999 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 124999 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 124999 or prevent reminders by adding the backport:skip label.

@orhantoy orhantoy removed backport missing Added to PRs automatically when the are determined to be missing a backport. auto-backport Deprecated - use backport:version if exact versions are needed labels Feb 28, 2022
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 124999 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 28, 2022
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 124999 or prevent reminders by adding the backport:skip label.

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 124999 or prevent reminders by adding the backport:skip label.

@orhantoy orhantoy added the backport:skip This commit does not require backporting label Mar 3, 2022
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orhantoy orhantoy orhantoy left review comments

@sphilipse sphilipse sphilipse approved these changes

@daveyholler daveyholler daveyholler approved these changes

Assignees

@byronhulcher byronhulcher

Labels
backport:skip This commit does not require backporting Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v8.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@byronhulcher @kibana-ci @kibanamachine @daveyholler @orhantoy @sphilipse