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

[ML] Adding cloud specific ML node warning #50139

Conversation

jgowdyelastic
Copy link
Member

@jgowdyelastic jgowdyelastic commented Nov 11, 2019

Adds a link to the cloud deployment when there are no nodes and kibana is running on cloud.

Text in callout

No ML nodes available
There are no ML nodes available.
You will not be able to create or run jobs.
Please edit your Elastic Cloud deployment and enable a machine learning node.

Can be tested by adding some settings to yml files:
elasticsearch.yml
node.ml: false

kibana.yml
xpack.cloud.id: <supply your own cloud id>

2019-10-31 14-10-55 2019-10-31 14_13_05

Also moves some code about:
Due to issues in the kibana CI, there some planned refactoring has not made it into this PR.
In a follow up PR, new_job_defaults.ts will be removed in favour of a dedicated ml_server_info service.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately
- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately

@jgowdyelastic jgowdyelastic added non-issue Indicates to automation that a pull request should not appear in the release notes :ml release_note:skip Skip the PR/issue when compiling release notes labels Nov 11, 2019
@jgowdyelastic jgowdyelastic requested a review from a team as a code owner November 11, 2019 10:40
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jgowdyelastic
Copy link
Member Author

retest

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💔 Build Failed

@poffdeluxe
Copy link
Contributor

poffdeluxe commented Nov 11, 2019

Hey there I was lookin into some of the test failures on this PR and noticed that whenever I try to access Kibana with this branch, I get an invalid regexp group syntax error. I believe it's related to this line: https://github.com/elastic/kibana/pull/50139/files#diff-7f047fa49a76cb2de3819ba4e6cbf3dfR73

@jgowdyelastic
Copy link
Member Author

thanks @poffdeluxe, looks like the regex was the issue.
Thanks for looking into it, it's quite strange it would be reported as a canvas test error.

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

@jgowdyelastic jgowdyelastic self-assigned this Nov 11, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jgowdyelastic
Copy link
Member Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

LGTM, some nit comments

export const NodeAvailableWarning: FC = () => {
if (mlNodesAvailable() === true || permissionToViewMlNodeCount() === false) {
return null;
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: else is redundant

return cloudInfo.isCloud;
}

export function cloudDeploymentId(): string | null {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: getCloudDeploymentId would be more descriptive


try {
const info = await callWithRequest('ml.info');
const cloudIdKey = 'xpack.cloud.id';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would be convenient to have a dictionary for such constants

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

LGTM

@jgowdyelastic
Copy link
Member Author

The comments raised will be addressed in a post 7.5 follow up PR which will also reintroduce the refactoring which was removed from original PR.

@jgowdyelastic jgowdyelastic merged commit 0e3237e into elastic:master Nov 12, 2019
@jgowdyelastic jgowdyelastic deleted the adding-cloud-specific-ml-node-warning-callout branch November 12, 2019 09:03
jgowdyelastic added a commit to jgowdyelastic/kibana that referenced this pull request Nov 12, 2019
* [ML] Adding cloud specific ML node warning

* fixing include

* adding callout and cloud checks

* fixing translations

* removing positive look behind in regex
jgowdyelastic added a commit to jgowdyelastic/kibana that referenced this pull request Nov 12, 2019
* [ML] Adding cloud specific ML node warning

* fixing include

* adding callout and cloud checks

* fixing translations

* removing positive look behind in regex
jgowdyelastic added a commit that referenced this pull request Nov 12, 2019
* [ML] Adding cloud specific ML node warning

* fixing include

* adding callout and cloud checks

* fixing translations

* removing positive look behind in regex
jgowdyelastic added a commit that referenced this pull request Nov 12, 2019
* [ML] Adding cloud specific ML node warning

* fixing include

* adding callout and cloud checks

* fixing translations

* removing positive look behind in regex
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 12, 2019
* upstream/master:
  [ML] Make navigation in tests more stable (elastic#50132)
  Migrate authorization subsystem to the new platform.  (elastic#46145)
  Bugfix: Interpreter conversion of string to number should throw on NaN elastic#27788 (elastic#50063)
  Update dependency @elastic/charts to v14 (elastic#49947)
  [ML] Adding cloud specific ML node warning (elastic#50139)
  Fixing bugs in the Shareable Runtime (elastic#49965)
  Revert router base name for Uptime plugin to use hash in default path. (elastic#50095)
  Ability to have telemetry always opted in (elastic#49798)
  Add "Get Help" and "Kibana Feedback" links to the help popover (elastic#49797)
  Removes references to Elasticsearch mapping types (elastic#47610)
  [skip-ci] Replace coordinate map in Kibana getting started docs with Maps (elastic#50167)
  [ML] Indicate missing required privileges for import in File Data Viz (elastic#50147)
  [SIEM][Detection Engine] Removes technical debt and minor bug fixes (elastic#50111)
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 12, 2019
* upstream/master:
  Move table-list-view to kibana-react (elastic#50046)
  [ML] Stats bar for data frame analytics (elastic#49464)
  [ML] Make navigation in tests more stable (elastic#50132)
  Migrate authorization subsystem to the new platform.  (elastic#46145)
  Bugfix: Interpreter conversion of string to number should throw on NaN elastic#27788 (elastic#50063)
  Update dependency @elastic/charts to v14 (elastic#49947)
  [ML] Adding cloud specific ML node warning (elastic#50139)
  Fixing bugs in the Shareable Runtime (elastic#49965)
  Revert router base name for Uptime plugin to use hash in default path. (elastic#50095)
  Ability to have telemetry always opted in (elastic#49798)
  Add "Get Help" and "Kibana Feedback" links to the help popover (elastic#49797)
  Removes references to Elasticsearch mapping types (elastic#47610)
  [skip-ci] Replace coordinate map in Kibana getting started docs with Maps (elastic#50167)
  [ML] Indicate missing required privileges for import in File Data Viz (elastic#50147)
  [SIEM][Detection Engine] Removes technical debt and minor bug fixes (elastic#50111)
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 12, 2019
…skip ci]

* upstream/master:
  Move table-list-view to kibana-react (elastic#50046)
  [ML] Stats bar for data frame analytics (elastic#49464)
  [ML] Make navigation in tests more stable (elastic#50132)
  Migrate authorization subsystem to the new platform.  (elastic#46145)
  Bugfix: Interpreter conversion of string to number should throw on NaN elastic#27788 (elastic#50063)
  Update dependency @elastic/charts to v14 (elastic#49947)
  [ML] Adding cloud specific ML node warning (elastic#50139)
  Fixing bugs in the Shareable Runtime (elastic#49965)
  Revert router base name for Uptime plugin to use hash in default path. (elastic#50095)
  Ability to have telemetry always opted in (elastic#49798)
  Add "Get Help" and "Kibana Feedback" links to the help popover (elastic#49797)
  Removes references to Elasticsearch mapping types (elastic#47610)
  [skip-ci] Replace coordinate map in Kibana getting started docs with Maps (elastic#50167)
  [ML] Indicate missing required privileges for import in File Data Viz (elastic#50147)
  [SIEM][Detection Engine] Removes technical debt and minor bug fixes (elastic#50111)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:ml non-issue Indicates to automation that a pull request should not appear in the release notes release_note:skip Skip the PR/issue when compiling release notes v7.5.0 v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants