-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[ML] Versioning file upload APIs #158265
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] Versioning file upload APIs #158265
Conversation
|
Pinging @elastic/ml-ui (:ml) |
nreese
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
code review only
nreese
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
code review only
| ) | ||
| .auth(IMPORTER_USER_NAME, IMPORT_USER_PASSWORD) | ||
| .set('kbn-xsrf', 'kibana') | ||
| .set('version', '1') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ELASTIC_HTTP_VERSION_HEADER from import { ELASTIC_HTTP_VERSION_HEADER } from '@kbn/core-http-common'; instead of version.
http.fetch converts version to ELASTIC_HTTP_VERSION_HEADER but this does not occur in integration tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot, thanks. This is correct in our previous versioning PRs, but I missed it here.
Updated in 957bbca
nreese
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ELASTIC_HTTP_VERSION_HEADER from import { ELASTIC_HTTP_VERSION_HEADER } from '@kbn/core-http-common'; instead of version in integration tests
peteharverson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and LGTM.
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
alvarezmelissa87
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ⚡
Adds versioning to all of the file upload APIs.
Versions are added to the server side routes and to the client side functions which call the routes.
Updates API tests to add the API version to the request headers.
All of the APIs are internal and have been given the version '1'.
Also renames
/internal/file_data_visualizer/analyze_fileto/internal/file_upload/analyze_fileIt appears this was a mistake from when the route was moved from the data visualiser plugin midway through development on this PR.
Internal APIs
/internal/file_upload/analyze_file/internal/file_upload/has_import_permission/internal/file_upload/index_exists/internal/file_upload/time_field_range