-
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
[console_extensions] disable when console is disabled #18673
Conversation
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.
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
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.
^ Crap, that was me on a different account....
LGTM
💔 Build Failed |
💚 Build Succeeded |
Doesn’t this mean you can no longer disable console_extensions alone? If so, was that intended? |
Good catch, pushed an update. |
6496203
to
b5f632e
Compare
💚 Build Succeeded |
isEnabled(config) { | ||
return ( | ||
config.get('console_extensions.enabled') && | ||
config.has('console.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.
Why do you need a call to has
for console
but not for console_extensions
?
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.
Our tests load x-pack plugins in isolation, and if there's no console to go along with console_extensions it'll error out. I was trying to find somewhere to mock console, but I decided it's a moderately legitimate check.
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 didn't even answer your question, doh. I didn't do has for console_extensions because I defined it in the schema below.
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.
Makes sense
LGTM |
* [console_extensions] disable when console is disabled * [console extensions] has console and enabled * [console_extensions] fix console_extensions.enabled
* [console_extensions] disable when console is disabled * [console extensions] has console and enabled * [console_extensions] fix console_extensions.enabled
Closes #18671.
To test, set
console.enabled
to false. The server should start with no errors.Without this change,
is expected on server start