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

[ML] NP Server: make security and spaces plugins optional #59156

Merged
merged 2 commits into from
Mar 5, 2020
Merged

[ML] NP Server: make security and spaces plugins optional #59156

merged 2 commits into from
Mar 5, 2020

Conversation

alvarezmelissa87
Copy link
Contributor

Summary

Ensure ml plugin works with security disabled. Moves security and spaces plugins from required to optional in kibana.json

@alvarezmelissa87 alvarezmelissa87 self-assigned this Mar 3, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

jgowdyelastic
jgowdyelastic approved these changes Mar 3, 2020
View reviewed changes
Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

Added a couple of comments but LGMT

x-pack/plugins/ml/server/plugin.ts Outdated
@@ -103,7 +103,7 @@ export class MlServerPlugin {
getLicenseCheckResults: () => this.licenseCheckResults,
};

annotationRoutes(routeInit, plugins.security);
annotationRoutes(routeInit, plugins?.security);
Copy link
Member

Choose a reason for hiding this comment

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

plugins won't be undefined here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch - fixed in 6fdc3375d75d9381b53e972c7a484d13b3dfc514

x-pack/plugins/ml/server/types.ts Outdated
@@ -24,16 +24,16 @@ export interface LicenseCheckResult {

export interface SystemRouteDeps {
cloud: CloudSetup;
spacesPlugin: SpacesPluginSetup;
spacesPlugin?: SpacesPluginSetup;
Copy link
Member

Choose a reason for hiding this comment

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

i just noticed that we use spacesPlugin here and spaces in PluginsSetup to refer to SpacesPluginSetup
but we use cloud in both to refer to CloudSetup.
It would be nice to have consistent naming for all plugin dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

but i'm not suggesting we change it in this PR. it's a bit out of scope

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a small change as it's only used in one of the route files. Updated here: 6fdc3375d75d9381b53e972c7a484d13b3dfc514

x-pack/plugins/ml/server/routes/annotations.ts Outdated

const currentUser =
securityPlugin !== undefined && securityPlugin.authc.getCurrentUser(request);
const user = currentUser ? currentUser : {};
Copy link
Member

Choose a reason for hiding this comment

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

looking at the User type, username isn't optional, so this check could probably be
const username = currentUser?.username || ANNOTATION_USER_UNKNOWN;

Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer the ?? operator, even if it does the same here.
const username = currentUser?.username ?? ANNOTATION_USER_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.

Good point - updated 6fdc3375d75d9381b53e972c7a484d13b3dfc514

@alvarezmelissa87 alvarezmelissa87 added the release_note:skip Skip the PR/issue when compiling release notes label Mar 3, 2020
@alvarezmelissa87 alvarezmelissa87 force-pushed the ml-update-required-plugins branch from 545409c to ca7f16e Compare March 4, 2020 15:06
@alvarezmelissa87 alvarezmelissa87 requested a review from a team as a code owner March 4, 2020 15:06
jgowdyelastic
jgowdyelastic approved these changes Mar 4, 2020
View reviewed changes
Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

Added a comment to remove a comment, but otherwise
LGTM

x-pack/plugins/ml/server/routes/annotations.ts
@@ -103,10 +103,10 @@ export function annotationRoutes(
const { indexAnnotation } = annotationServiceProvider(context);

const currentUser =
securityPlugin !== undefined && securityPlugin.authc.getCurrentUser(request);
const user = currentUser ? currentUser : {};
securityPlugin !== undefined ? securityPlugin.authc.getCurrentUser(request) : {};
// @ts-ignore username doesn't exist on {}
Copy link
Member

Choose a reason for hiding this comment

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

looks like this @ts-ignore can go now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That warning still shows up since currentUser can still be {} 😞

Copy link
Member

@jgowdyelastic jgowdyelastic Mar 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

ah ok. i missed that.
would it be worth changing the line above to make currentUser null or undefined in that situation?
As it would be caught by currentUser?.username ?? on the line below.

peteharverson
peteharverson approved these changes Mar 4, 2020
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.

Tested and LGTM

@alvarezmelissa87
Copy link
Contributor Author

retest

@alvarezmelissa87 alvarezmelissa87 force-pushed the ml-update-required-plugins branch from 6fdc337 to 77685dd Compare March 4, 2020 19:10
@alvarezmelissa87 alvarezmelissa87 force-pushed the ml-update-required-plugins branch from 77685dd to 9759791 Compare March 4, 2020 22:07
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

  • 💔 Build #30858 failed 77685ddf3ddb12083139b8a979d578c5e0b03f2e
  • 💚 Build #30843 succeeded 6fdc3375d75d9381b53e972c7a484d13b3dfc514
  • 💚 Build #30782 succeeded ca7f16e173516ab5b5a7b5e11c628293cda94516
  • 💚 Build #30395 succeeded 545409c4cadb742988b7db2708e6b49395ec18c1

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

@alvarezmelissa87 alvarezmelissa87 merged commit d55524c into elastic:master Mar 5, 2020
@alvarezmelissa87 alvarezmelissa87 mentioned this pull request Mar 5, 2020
Merged
@alvarezmelissa87 alvarezmelissa87 deleted the ml-update-required-plugins branch March 5, 2020 15:07
alvarezmelissa87 added a commit to alvarezmelissa87/kibana that referenced this pull request Mar 5, 2020
@alvarezmelissa87
[ML] NP Server: make security and spaces plugins optional (elastic#59156
f0f6d73 
)

* make security and spaces plugins optional

* update spacesPlugin name. update current user check
alvarezmelissa87 added a commit that referenced this pull request Mar 6, 2020
@alvarezmelissa87
[ML] NP Server: make security and spaces plugins optional (#59156) (#…
88d4633 
…59434)

* make security and spaces plugins optional

* update spacesPlugin name. update current user check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgowdyelastic jgowdyelastic jgowdyelastic approved these changes

@peteharverson peteharverson peteharverson approved these changes

@pgayvallet pgayvallet pgayvallet approved these changes

Assignees

@alvarezmelissa87 alvarezmelissa87

Labels
Feature:NP Migration :ml release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@alvarezmelissa87 @elasticmachine @kibanamachine @pgayvallet @peteharverson @jgowdyelastic