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

Spaces - migrate default space and enter space view to KP #66098

Merged
merged 3 commits into from
May 12, 2020

Conversation

legrego
Copy link
Member

@legrego legrego commented May 11, 2020

Summary

Migrates the following from the LP to the KP:

  • /spaces/enter view. Used by the space selector UIs to send users to the appropriate default route.
  • Creation of the default space. This takes advantage of the CoreStatus service, which allows us to detect when it is "safe" to attempt default space creation.

@legrego legrego marked this pull request as ready for review May 12, 2020 11:45
@legrego legrego requested a review from a team as a code owner May 12, 2020 11:45
@legrego legrego added 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.9.0 v8.0.0 labels May 12, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@@ -0,0 +1,50 @@
/*
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 likely overkill for the needs of this PR, but I wanted to set up Spaces to have a similar license service as Security, for consistency. If Spaces ever gets more complex, we'll already be setup to support it.

const { savedObjects } = coreMock.createStart();
const repository = savedObjects.createInternalRepository() as jest.Mocked<SavedObjectsRepository>;
// simulate space not found
repository.get.mockRejectedValue(SavedObjectsErrorHelpers.createGenericNotFoundError());
Copy link
Member Author

Choose a reason for hiding this comment

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

note I don't love that I'm asserting the behavior of a dependency of a sub-module in these tests, but it seemed like the "safest" thing to do for now. The default space service is inherently coupled to the repository given the work it has to do.

I could jest.mock the create_default_space module, but I didn't love that idea either.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd normally lean towards mocking the sub-module, but this seems fine for this suite of unit tests. Like you said, it's inherently coupled.

summary: 'not initialized',
} as ServiceStatus);

this.initializeSubscription = combineLatest([coreStatus.core$, license$])
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 rxjs flow was taken/inspired by the legacy watch_status_and_license_to_initialize.js utility

.subscribe();

return {
serviceStatus$: this.serviceStatus$!.asObservable(),
Copy link
Member Author

Choose a reason for hiding this comment

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

note for now, this status is only consumed by the unit tests for this module, but once the KP has a proper plugin status service, this will be used to inform the platform about the overall health of the Spaces plugin.

Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

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

I had a minor nit to update a comment.

Changes tested locally and works great. LGTM!

const { savedObjects } = coreMock.createStart();
const repository = savedObjects.createInternalRepository() as jest.Mocked<SavedObjectsRepository>;
// simulate space not found
repository.get.mockRejectedValue(SavedObjectsErrorHelpers.createGenericNotFoundError());
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd normally lean towards mocking the sub-module, but this seems fine for this suite of unit tests. Like you said, it's inherently coupled.

@legrego
Copy link
Member Author

legrego commented May 12, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@legrego legrego merged commit dc8dd19 into elastic:master May 12, 2020
@legrego legrego deleted the spaces/np-final-push branch May 12, 2020 23:59
legrego added a commit to legrego/kibana that referenced this pull request May 13, 2020
v1v added a commit to v1v/kibana that referenced this pull request May 13, 2020
* upstream/master: (223 commits)
  [Ingest] Support root level yaml variables in agent stream template (elastic#66120)
  [Snapshot Restore] Fix error when deleting snapshots behind reverse proxy (elastic#66147)
  [Lens] fix empty state for pie (elastic#66206)
  [APM] Improve e2e tests (elastic#66373)
  [ML] Data Frame Analytics: Fix steps to be named phases. (elastic#65855)
  [Discover] Encode context link filter part (elastic#63831)
  [APM] Scope APM alert creation to environment (elastic#65681)
  Move Kibana Usage collectors outside the telemetry plugin (elastic#65663)
  [ML] Data Frame Analytics: Fix confusion matrix data grid width. (elastic#65818)
  Switch to core application service (elastic#63443)
  Removes use of prefer_v2_templates (elastic#66316)
  [Maps] handle case where fit to bounds does not match any documents (elastic#66307)
  log error instead of throw (elastic#66254)
  [plugin-helpers] remove outdated postinstall task (elastic#66324)
  Spaces - migrate default space and enter space view to KP (elastic#66098)
  [APM] Change plugin id for `apm_oss` to `apmOss` (elastic#66164)
  [Maps] convert map_selectors to TS (elastic#65905)
  [uptime/usage-collector] add missing await (elastic#66079)
  [Ingest] Add additional attributes to the Datasources Saved Object (elastic#66127)
  [Endpoint]EMT-339: add new policy response schema (elastic#66264)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants