-
Notifications
You must be signed in to change notification settings - Fork 799
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
[backend/frontend] Fix ingestion creation (#4020) #4042
Conversation
+ '}' | ||
+ 'ctx._source.remove("current_state")', | ||
params: { | ||
emptyCurrentState: {}, |
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.
Should not be as string here? '{}'
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.
We tested it against problematic stored document and the migration executed flawlessly. What benefit do you see to make it a string?
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.
no it's an empty object, not a string
|
||
export const up = async (next) => { | ||
const defaultDate = new Date(); | ||
defaultDate.setHours(0, 0, 0, 0); |
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.
Using now() is not sufficient? Why round on hour?
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.
I guess it is, though we replicated this behavior from the creation process for compliance.
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.
yes we used the same default date as the one used in SyncCreation (dayStartDate()
)
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 : ✅ Everything works fine for the ingestion page.
I have just notice some minor stuff :
- The banner message is not tranlasted
- There is no gap between "stream ID" and "running", I think it's not really pretty (even if it was already the case)
- The button "start" and "stop" does not have the right color
…_date to enforce date type
fe18643
to
996e226
Compare
Proposed changes
Related issues
Checklist
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...