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] Add kibana setting for file data visualizer max file size #64427

Conversation

jgowdyelastic
Copy link
Member

Configuring the max file upload size is now done via a setting in the Advanced Settings page of kibana's management app.

Reverts some of the changes made in #62752

image

Basic validation looks for a data string using a regex. max and min testing is not possible.

image

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

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. Just one typo to correct I think.

Is this edit to the docs still valid now that the setting has been moved into the advanced settings page? #64006

x-pack/plugins/ml/server/lib/register_settings.ts Outdated Show resolved Hide resolved
@jgowdyelastic
Copy link
Member Author

retest

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

coreSetup.uiSettings.register({
[FILE_DATA_VISUALIZER_MAX_FILE_SIZE]: {
name: i18n.translate('xpack.ml.maxFileSizeSettingsName', {
defaultMessage: 'File Data Visualizer maximum file upload size',
Copy link
Contributor

Choose a reason for hiding this comment

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

In the docs we don't differentiate between the "Data Visualizer" and the "File Data Visualizer" (I think it's because they're both on the same "Data Visualizer" tab). IMO it's not necessary to have the "File" qualifier here either:

Suggested change
defaultMessage: 'File Data Visualizer maximum file upload size',
defaultMessage: 'Data Visualizer maximum file upload size',

value: MAX_FILE_SIZE,
description: i18n.translate('xpack.ml.maxFileSizeSettingsDescription', {
defaultMessage:
'Sets the file size limit when importing data in the File Data Visualizer. The highest supported value for this setting is 1GB.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto re "File Data Visualizer":

Suggested change
'Sets the file size limit when importing data in the File Data Visualizer. The highest supported value for this setting is 1GB.',
'Sets the file size limit when importing data in the Data Visualizer. The highest supported value for this setting is 1GB.',

* you may not use this file except in compliance with the Elastic License.
*/

export const FILE_DATA_VISUALIZER_MAX_FILE_SIZE = 'ml:fileDataVisualizerMaxFileSize';
Copy link
Contributor

Choose a reason for hiding this comment

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

Not mandatory, but if you follow my other suggestions, it might be a good idea to synch here too:

Suggested change
export const FILE_DATA_VISUALIZER_MAX_FILE_SIZE = 'ml:fileDataVisualizerMaxFileSize';
export const FILE_DATA_VISUALIZER_MAX_FILE_SIZE = 'ml:DataVisualizerMaxFileSize';


[horizontal]
`ml:fileDataVisualizerMaxFileSize`:: Sets the file size limit when importing
Copy link
Contributor

Choose a reason for hiding this comment

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

If you accept my suggestion about changing the setting name, it will need to be updated here too:

Suggested change
`ml:fileDataVisualizerMaxFileSize`:: Sets the file size limit when importing
`ml:DataVisualizerMaxFileSize`:: Sets the file size limit when importing

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

LGTM ⚡

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@jgowdyelastic jgowdyelastic merged commit f9c81a3 into elastic:master Apr 28, 2020
@jgowdyelastic jgowdyelastic deleted the adding-kibana-setting-for-max-file-upload branch April 28, 2020 08:22
jgowdyelastic added a commit to jgowdyelastic/kibana that referenced this pull request Apr 28, 2020
…ic#64427)

* [ML] Add kibana setting for file data visualizers max file size

* adding failsafe for setting

* fixing id

* [DOCS] Updates Data Visualizer setting

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: lcawl <[email protected]>
jgowdyelastic added a commit that referenced this pull request Apr 28, 2020
… (#64624)

* [ML] Add kibana setting for file data visualizers max file size

* adding failsafe for setting

* fixing id

* [DOCS] Updates Data Visualizer setting

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: lcawl <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: lcawl <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants