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

Spaces - server-side to NP plugin #46181

Merged
merged 34 commits into from
Oct 15, 2019
Merged

Spaces - server-side to NP plugin #46181

merged 34 commits into from
Oct 15, 2019
+2,736 −2,324

Conversation

legrego
Copy link
Member

@legrego legrego commented Sep 19, 2019 •
edited
Loading

Summary

This moves the bulk of the Spaces server-side implementation to a NP plugin.

TODO

deferring

@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

This was referenced Sep 23, 2019
Merged
Closed
@legrego legrego mentioned this pull request Sep 25, 2019
Merged
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@legrego legrego requested a review from azasypkin October 8, 2019 19:45
@azasypkin
Copy link
Member

ACK: will be reviewing tomorrow

azasypkin
azasypkin reviewed Oct 10, 2019
View reviewed changes
Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

Finished with the code review, looks great so far! Haven't tested locally yet, but wanted to leave a code feedback already, nothing major - mostly nits.

x-pack/legacy/plugins/spaces/index.ts Outdated Show resolved Hide resolved
x-pack/plugins/spaces/server/config.ts Outdated Show resolved Hide resolved
x-pack/plugins/spaces/server/spaces_service/spaces_service.ts
import { getSpaceIdFromPath, addSpaceIdToPath } from '../../common/lib/spaces_url_parser';
import { DEFAULT_SPACE_ID } from '../../common/constants';
import { spaceIdToNamespace, namespaceToSpaceId } from '../lib/utils/namespace';
import { Space } from '../../common/model/space';

type RequestFacade = KibanaRequest | Legacy.Request;
Copy link
Member

Choose a reason for hiding this comment

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

question: how do you feel about dropping support for Legacy.Request completely in NP plugin? If someone in LP wants to call request-based NP API they will have to call KibanaRequest.from(legacyRequest). We also have legacy Spaces plugin that would do that internally for the APIs it still wants to re-expose to LP.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's a great idea!

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I'm going to revert this change - the spaces service still requires the getBasePath function that exists on the LP request and the "fake" requests used by alerting and reporting. I'd rather come up with a holistic solution for these fake requests in a dedicated PR since it will likely end up touching these other plugins as well

Copy link
Member

@azasypkin azasypkin Oct 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Ugh, I see 😭 Tackling this separately sounds good to me.

x-pack/plugins/spaces/server/plugin.ts Outdated Show resolved Hide resolved
x-pack/plugins/spaces/server/lib/toggle_ui_capabilities.test.ts
@@ -5,7 +5,7 @@
*/
Copy link
Member

Choose a reason for hiding this comment

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

note: just an observation, we have a bunch of lib folders (and even one lib/utils) here and there that basically serve as catch-all buckets for the code we couldn't properly categorize. Not in this PR, but it feels like a good time to think how we can have a better structure for the content of the lib folders.

x-pack/plugins/spaces/server/lib/get_spaces_usage_collector.ts Show resolved Hide resolved
x-pack/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.ts Outdated Show resolved Hide resolved
x-pack/plugins/spaces/server/lib/request_interceptors/on_request_interceptor.ts Outdated Show resolved Hide resolved
x-pack/plugins/spaces/server/routes/api/__fixtures__/create_legacy_api.ts Outdated Show resolved Hide resolved
x-pack/plugins/spaces/server/routes/api/__fixtures__/create_legacy_api.ts Outdated Show resolved Hide resolved
@legrego legrego removed the request for review from kobelb October 10, 2019 16:44
@legrego legrego requested a review from a team October 11, 2019 12:01
@legrego legrego requested a review from a team as a code owner October 11, 2019 12:01
@legrego
Copy link
Member Author

legrego commented Oct 11, 2019

@elastic/kibana-stack-services @elastic/ml-ui You've been pinged due to an interface rename from SpacesPlugin to LegacySpacesPlugin. There should be no functional changes that concern you, but you're of course free to take a look if you'd like.

peteharverson
peteharverson approved these changes Oct 11, 2019
View reviewed changes
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

ML edit LGTM!

@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@legrego
Copy link
Member Author

legrego commented Oct 11, 2019

@azasypkin ready for another review whenever you get a chance!

@azasypkin
Copy link
Member

ACK: reviewing

azasypkin
azasypkin approved these changes Oct 14, 2019
View reviewed changes
Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

LGTM, great job! Performed a smoke test locally and haven't noticed anything suspicious.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@legrego legrego merged commit 7fd639b into elastic:master Oct 15, 2019
@legrego legrego deleted the np/spaces-server-1 branch October 15, 2019 13:17
@legrego legrego mentioned this pull request Oct 15, 2019
Merged
legrego added a commit to legrego/kibana that referenced this pull request Oct 15, 2019
@legrego
Spaces - server-side to NP plugin (elastic#46181)
6afd369
legrego added a commit that referenced this pull request Oct 15, 2019
@legrego
[7.x] Spaces - server-side to NP plugin (#46181) (#48222)
30f69df
@tylersmalley tylersmalley mentioned this pull request Dec 10, 2019
Closed
1 task
@legrego legrego mentioned this pull request Jan 24, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eliperelman eliperelman eliperelman left review comments

@peteharverson peteharverson peteharverson approved these changes

@azasypkin azasypkin azasypkin approved these changes

Assignees
No one assigned
Labels
Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.5.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@legrego @elasticmachine @azasypkin @eliperelman @peteharverson