-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 ability to pass configs in and fixing misc bugs #7414
Conversation
* limitations under the License. | ||
*/ | ||
|
||
window.consoleConfig = { /* future configs may go 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.
I think you should add hideLegacy
and baseURL
here maybe with their default values and an comment explanations of what they do since they seem more like 'now' configs than 'future' configs?
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.
Since this gets built into the Druid package, it's not really configurable unless you build your own package. Would it make sense as a follow up to improve the Druid side that serves this path to optionally wire up to a Druid config option that points to an alternate file to use for console-config.js
so that an operator might supply their own js file with config values to override the defaults?
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 am imagining that this would be a default and there would be a Druid property that has a JSON object it in that Druid will stringily and prepend with window.consoleConfig =
and send instead of this file.
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 do not see a need to add default configs here as this IMO is just a stepping stone PR towards what I said above. I will add a sweet interface for it though.
web-console/src/entry.ts
Outdated
ReactDOM.render( | ||
React.createElement( | ||
ConsoleApplication, | ||
{ | ||
version: '0.0.1' | ||
version: '0.15.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.
Hmm, it would be very nice to find a more elegant way to sync versions from Druid's pom.xml to the web console, but I don't really know what that is right now (and not really necessary to address in this PR i think)
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.
yeah... to be honest this is dead code. The version here is never used, the console makes a GET /status
query to get the version. Removing.
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.
🤘
* Adding ability to pass configs in and fixing misc bugs * update lock file * remove dead version param
Adding a rudimentary mechanism for Druid to pass configs to the console (by overriding the
web-console/console-config.js
file) for future extensibility.Also making misc fixes:
addOnBlur
for tag inputInitialize lookups
on errors other than 404 and will correctly display a legitimate errordisplay: none
to hide elementsLookup delete confirmation: