-
Notifications
You must be signed in to change notification settings - Fork 58
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
Adding remote indices and multi index functionality #854
Conversation
f44e719
to
9c6dfb1
Compare
) => { | ||
const newSelectedOptionsSet = new Set(newSelectedOptions); | ||
const indexRemoved: boolean = | ||
!(newSelectedOptions && oldSelectedOptions) || |
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.
If both newSelectedOptions and oldSelectedOptions are null or undefined, would it make more sense for isSelectedOptionIndexRemoved to return false instead of true, because if both are null or undefined, there is no change in the selection state — technically, no option was "removed." ?
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.
Originally if both were null it was okay for me to set it to true because then it would also make sense to reset the model configuration if we can't find any selected indices.
I think actually though we pretty much never hit the case where both are undefined so it wasn't making too much of an effect and it logically makes more sense to say indexRemoved is false in that case and if we got to this point we would have had an indexRemoved previously.
I tested it out and removing !(newSelectedOptins && oldSelectedOptions)
and adding:
if (_.isEmpty(oldSelectedOptions) && _.isEmpty(newSelectedOptions)) {
return false;
}
also works
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.
make sense
cypress test is failing. Can you either fix it or make sure it passes locally? |
It does pass locally, I'm not sure why its failing here. |
Cypress test failed with a different reason now: plugins/anomaly-detection-dashboard 03:56 4 3 1 - - │ |
I think I fixed the timeout error during historical cypress tests in #859 and opensearch-project/opensearch-dashboards-functional-test#1537 Not sure about events_spec.js cypress failure above. |
}); | ||
remoteMappings = convertFieldCapsToMappingStructure(fieldCapsResponse); | ||
} | ||
Object.assign(mappings, remoteMappings); |
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.
Does this Object.assign works fine on merging nested structure? Should we create a new map object instead of mutating the mappings
variable directly?
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 was reading online that Object.assign is more efficient which can be important as we can have indices with very large mappings. I tested it on complicated nested structures
overall lgtm, just I see E2E CI is failing. Please look into that to make sure it won't be a RC build blocker |
Signed-off-by: Amit Galitzky <[email protected]>
Signed-off-by: Amit Galitzky <[email protected]>
8c4f393
to
994775a
Compare
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-854-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b26f8c32489693d36ac5479c99a7d9f1d43ef01d
# Push it to GitHub
git push --set-upstream origin backport/backport-854-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x Then, create a pull request where the |
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.17 2.17
# Navigate to the new working tree
cd .worktrees/backport-2.17
# Create a new branch
git switch --create backport/backport-854-to-2.17
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b26f8c32489693d36ac5479c99a7d9f1d43ef01d
# Push it to GitHub
git push --set-upstream origin backport/backport-854-to-2.17
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.17 Then, create a pull request where the |
…ct#854) * Adding remote indices and multi index functionality Signed-off-by: Amit Galitzky <[email protected]> * changing removed index check Signed-off-by: Amit Galitzky <[email protected]> --------- Signed-off-by: Amit Galitzky <[email protected]>
(cherry picked from commit 4942f5f) Co-authored-by: Amit Galitzky <[email protected]>
Description
Issues Resolved
resolves #856
resolves #855
Check List
--signoff
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.