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

[Enterprise Search] Added an App Search route for listing Credentials #75487

Merged
merged 17 commits into from
Aug 27, 2020
Merged

[Enterprise Search] Added an App Search route for listing Credentials #75487

merged 17 commits into from
Aug 27, 2020

Conversation

JasonStoltz
Copy link
Member

Summary

http://localhost:5601/api/app_search/credentials/collection?page[current]=1

In addition to a route for listing Credentials, this also adds a
utility function which helps create API routes which simply proxy
the App Search API.

The reasoning for this is as follows;

  1. Creating new routes takes less effort and cognitive load if we
    can simply just create proxy routes that use the APIs as is.
  2. It keeps the App Search API as the source of truth. All logic is
    implemented in the underlying API.
  3. It makes unit testing routes much simpler. We do not need to verify
    any connectivity to the underlying App Search API, because that is
    already tested as part of the utility.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@JasonStoltz JasonStoltz requested a review from cee-chen August 19, 2020 19:46
@JasonStoltz JasonStoltz added release_note:skip Skip the PR/issue when compiling release notes Feature:Plugins v7.10.0 labels Aug 19, 2020
cee-chen
cee-chen reviewed Aug 20, 2020
View reviewed changes
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Some comments addressing the type errors - if you want to test locally as well, you can run node scripts/type_check.js --project x-pack/tsconfig.json

x-pack/plugins/enterprise_search/server/routes/app_search/app_search_request_handler.ts Outdated Show resolved Hide resolved
x-pack/plugins/enterprise_search/server/routes/app_search/app_search_request_handler.ts Outdated Show resolved Hide resolved
@cee-chen
Copy link
Contributor

Hmm, looks like there's even more type shenanigans going on now, sorry 😞 Let me know if want to pair or rubber ducky or anything, I can be around!

The unit tests I think can be fixed by changing responseMock's notFound to customError:
https://github.com/elastic/kibana/blob/0c34d890c0e023c099f35f3097c1f395c68c0114/x-pack/plugins/enterprise_search/server/routes/app_search/app_search_request_handler.test.ts#L22-L25

@JasonStoltz
Copy link
Member Author

Yeah, I was hoping that my editor would catch all the TS issues but looks like it didn't 😢 . I'll get these fixed up, thanks.

@JasonStoltz
Copy link
Member Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

merge conflict between base and head

@cee-chen cee-chen mentioned this pull request Aug 24, 2020
Merged
8 tasks
@JasonStoltz JasonStoltz force-pushed the credentials-endpoint branch 2 times, most recently from 3716ed2 to 6b6742a Compare August 24, 2020 19:09
@JasonStoltz JasonStoltz requested a review from cee-chen August 24, 2020 21:21
@JasonStoltz
Copy link
Member Author

@constancecchen This is ready for a second look.

cee-chen
cee-chen reviewed Aug 25, 2020
View reviewed changes
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Naming comments - I think we can safely change all AppSearch name references to EnterpriseSearch

x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts Outdated Show resolved Hide resolved
x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts Outdated Show resolved Hide resolved
x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts Outdated Show resolved Hide resolved
x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts Outdated Show resolved Hide resolved
x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts Outdated Show resolved Hide resolved
x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts Outdated Show resolved Hide resolved
x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts Outdated Show resolved Hide resolved
x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts Outdated Show resolved Hide resolved
x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts Outdated Show resolved Hide resolved
x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts Outdated Show resolved Hide resolved
cee-chen
cee-chen reviewed Aug 26, 2020
View reviewed changes
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Last set of comments I think (mostly just super small nits!) Thanks so much for addressing all my feedback! 🙇‍♀️

x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts Outdated Show resolved Hide resolved
x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts Outdated Show resolved Hide resolved
x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts Outdated Show resolved Hide resolved
x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts Outdated Show resolved Hide resolved
x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts Outdated Show resolved Hide resolved
JasonStoltz and others added 16 commits August 26, 2020 15:14
@JasonStoltz
Added an App Search route for listing Credentials
5ca4fcc 
In addition to a route for listing Credentials, this also adds a
utility function which helps create API routes which simply proxy
the App Search API.

The reasoning for this is as follows;
1. Creating new routes takes less effeort and cognitive load if we
can simply just create proxy routes that use the APIs as is.
2. It keeps the App Search API as the source of truth. All logic is
implemented in the underlying API.
3. It makes unit testing routes much simpler. We do not need to verify
any connectivity to the underlying App Search API, because that is
already tested as part of the utility.
@JasonStoltz
Apply suggestions from code review
99cbfcf 
Co-authored-by: Constance <constancecchen@users.noreply.github.com>
@JasonStoltz
Fixed failing specs
d3c3135
@JasonStoltz
Updated createAppSearchRequestHandler test
2f234f3
@JasonStoltz
Made hasValidData optional
634a6ec
@JasonStoltz
Rename createAppSearchRequestHandler
d7e512c
@JasonStoltz
Changed credentials API route
1470eaf
@JasonStoltz
Fixed TS errors
80df86c
@JasonStoltz
Fixed another type error
0eaf5e0
@JasonStoltz
Apply suggestions from code review
5bc3ed9 
Co-authored-by: Constance <constancecchen@users.noreply.github.com>
@JasonStoltz
Get working again after feedback fixes
e936944
@cee-chen @JasonStoltz
Further DRY out Kibana auth header
576f562
@JasonStoltz
Implementing PR feedback
f533767
@JasonStoltz
Totally botched a conflict merge
f02285b
@JasonStoltz
Eslint
fc52ba4
@JasonStoltz
Apply suggestions from code review
33b05a7 
Co-authored-by: Constance <constancecchen@users.noreply.github.com>
@JasonStoltz JasonStoltz force-pushed the credentials-endpoint branch from eba1cfd to 33b05a7 Compare August 26, 2020 19:14
@JasonStoltz JasonStoltz requested a review from cee-chen August 26, 2020 19:15
cee-chen
cee-chen approved these changes Aug 26, 2020
View reviewed changes
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Woo hoo, I think this is good to go! Thanks so much for this amazing helper again - can't wait to use it!! 🎉

@cee-chen
Copy link
Contributor

Also as a quick reminder for after this PR gets merged in - you'll want to run yarn backport and backport this to the 7.x branch!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

distributable file count

id value diff baseline
total 53199 +2 53197

History

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

@JasonStoltz JasonStoltz merged commit f065191 into elastic:master Aug 27, 2020
@JasonStoltz JasonStoltz deleted the credentials-endpoint branch August 27, 2020 12:44
@JasonStoltz JasonStoltz mentioned this pull request Aug 27, 2020
Merged
JasonStoltz added a commit to JasonStoltz/kibana that referenced this pull request Aug 27, 2020
@JasonStoltz
[Enterprise Search] Added an App Search route for listing Credentials (…
8dad004 
…elastic#75487)

In addition to a route for listing Credentials, this also adds a
utility function which helps create API routes which simply proxy
the App Search API.

The reasoning for this is as follows;
1. Creating new routes takes less effort and cognitive load if we
can simply just create proxy routes that use the APIs as is.
2. It keeps the App Search API as the source of truth. All logic is
implemented in the underlying API.
3. It makes unit testing routes much simpler. We do not need to verify
any connectivity to the underlying App Search API, because that is
already tested as part of the utility.
JasonStoltz added a commit that referenced this pull request Aug 27, 2020
@JasonStoltz
[Enterprise Search] Added an App Search route for listing Credentials (…
6e624bf 
…#75487) (#76080)

In addition to a route for listing Credentials, this also adds a
utility function which helps create API routes which simply proxy
the App Search API.

The reasoning for this is as follows;
1. Creating new routes takes less effort and cognitive load if we
can simply just create proxy routes that use the APIs as is.
2. It keeps the App Search API as the source of truth. All logic is
implemented in the underlying API.
3. It makes unit testing routes much simpler. We do not need to verify
any connectivity to the underlying App Search API, because that is
already tested as part of the utility.
@cee-chen cee-chen mentioned this pull request Aug 27, 2020
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cee-chen cee-chen cee-chen approved these changes

Assignees
No one assigned
Labels
Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.10.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JasonStoltz @cee-chen @elasticmachine @kibanamachine