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
NP Migration: Sample data #52753
NP Migration: Sample data #52753
Conversation
…ver_tutorials_api
…orm tutorial plugin registrations
… tutorials service
… the home plugin public folder
…gin_home_feature_catalogue
…s api http routes for legacy platform and NP
Pinging @elastic/kibana-app (Team:KibanaApp) |
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.
Infra changes look good, thanks 👌
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.
ML changes LGTM
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.
Code LGTM 👍
|
||
// Set to true to move timestamp to current week, preserving day of week and time of day | ||
// Relative distance from timestamp to currentTimeMarker will not remain the same | ||
preserveDayOfWeekTimeOfDay: boolean; |
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.
Thx for all this context ❤️
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.
This goes to @TinaHeiligers - I just brought the PR home :)
logger: Logger | ||
) => { | ||
const bulkInsert = async (docs: any) => { | ||
function updateTimestamps(doc: any) { |
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.
Minor nit: it would help code readability at this position to e.g. refactor updateTimestamps
out of the bulkInsert
function, and to use an arrow function. would de-nest the code.
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 point, refactored this a bit.
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
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.
Canvas changes are good 👍
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.
Maps lgtm!
- code review
- tested locally on chrome
Fixes #50675
This PR migrates all APIs and route handlers around sample data to the new platform as part of the
home plugin
.Changes the status code of the uninstall route from
200
to204