-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
disabling ccr and remote clusters when index mgmt is disabled #32203
disabling ccr and remote clusters when index mgmt is disabled #32203
Conversation
Pinging @elastic/es-ui |
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.
Thanks for fixing this Bill! Had a few suggestions about the code.
config.has('xpack.index_management.enabled') && | ||
config.get('xpack.index_management.enabled') && | ||
config.has('xpack.remote_clusters.enabled') && | ||
config.get('xpack.remote_clusters.enabled') |
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.
It looks like index_management
and remote_clusters
are each being checked twice. I think we mean this?
return (
config.get('xpack.ccr.enabled') &&
config.has('xpack.index_management.enabled') &&
config.has('xpack.remote_clusters.enabled')
);
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.
"has" checks for the existence of the key and "get" checks for its value. "get' will blow up if has is false, hence the two checks.
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.
not true, it turns out. Tested get with nonexistent key and it worked fine.
config.has('xpack.remote_clusters.enabled') && | ||
config.get('xpack.remote_clusters.enabled') | ||
); | ||
}, |
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.
CCR's UI surfaces cross-links with Remote Clusters. Should we also set ccrUiEnabled
to false
on line 25 if Remote Clusters UI is disabled?
return {
ccrUiEnabled: (
config.get('xpack.ccr.ui.enabled')
&& config.get('xpack.remote_clusters.ui.enabled')
)
};
return ( | ||
config.get('xpack.remote_clusters.enabled') && | ||
config.has('xpack.index_management.enabled') && | ||
config.get('xpack.index_management.enabled') |
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.
Can we remove line 51 since it's a dupe of line 50?
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.
it's not AFAIK
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.
OK, turns out the "has" calls are not necessary. Saw this in someone else's code and assumed they were, but this doesn't crash Kibana: config.get('xpack.doesntexist.enabled')
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 actually had just scanned the code too quickly and didn't notice one was has
and one was get
. Looks like we both learned something new 😄
💚 Build Succeeded |
💔 Build Failed |
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! Thanks @bmcconaghy for this 👍
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! Tested locally:
xpack.index_management.enabled: false
also disables CCR and RCxpack.remote_clusters.enabled: false
also disables CCRxpack.remote_clusters.ui.enabled: false
also disables the CCR UI
No fatal errors.
Retest |
💔 Build Failed |
retest |
💚 Build Succeeded |
…c#32203) * disabling ccr and remote clusters when index mgmt is disabled * addressing PR feedback
…c#32203) * disabling ccr and remote clusters when index mgmt is disabled * addressing PR feedback
#32234) * disabling ccr and remote clusters when index mgmt is disabled * addressing PR feedback
…c#32203) * disabling ccr and remote clusters when index mgmt is disabled * addressing PR feedback
#32235) * disabling ccr and remote clusters when index mgmt is disabled * addressing PR feedback
#32236) * disabling ccr and remote clusters when index mgmt is disabled * addressing PR feedback
* disabling ccr and remote clusters when index mgmt is disabled * addressing PR feedback adjusting config paths for index_management
#32248) * disabling ccr and remote clusters when index mgmt is disabled * addressing PR feedback adjusting config paths for index_management
Closes #32160
This PR disables CCR and Remote Clusters when index management has been disabled. Additionally, it disables CCR when remote clusters has been disabled.