-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Support for dropdown of predefined options in data sources setup #4161
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.
See comment about the schema.
Will it pick some value by default? I think think it should pick either the default value as default and if no default is specified, then it should pick the first value.
"options": [ | ||
{"value": "beeswax"}, | ||
{"value": "hiveserver2"} | ||
], |
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.
This won't pass schema validation. A valid enum is:
{
"protocol": {
"type": "string",
"enum": ["beeswax", "hiveserver2"]
}
}
We might want to add our own extension to provide friendly names for options:
{
"protocol": {
"type": "string",
"extendedEnum": [{"value", "beeswax", "name": "Beeswax"}, {"value": "hiveserver2", "name": "Hive Server 2"}]
}
}
And we will transform extendedEnum
into enum
before passing it into jsonschema.validate
call.
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 might want to add our own extension to provide friendly names for options
That's exactly why I went with the list of objects on that, but yeah, it does make sense to permit a simplified version.
And we will transform extendedEnum into enum before passing it into jsonschema.validate call.
That's the only thing missing in terms of backend code?
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.
That's the only thing missing in terms of backend code?
I think so.
For now only if you specify a default value, for other cases I went with the placeholder. First value is an option too, I'll update that. |
Because it will fail validation if you don't specify a value from the list it's best if we just pick one for you. |
redash/utils/configuration.py
Outdated
@@ -24,6 +24,11 @@ def __init__(self, config, schema=None): | |||
self.set_schema(schema) | |||
|
|||
def set_schema(self, schema): | |||
if isinstance(schema, dict): |
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.
This is not needed: schema
is always a dict.
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 getting a "NoneType
has no attribute..." And that's why I put it 😬
👍 |
What type of PR is this? (check all applicable)
Description
This adds support to use
Select
in Data sources/Destinations setup.Related Tickets & Documents
--
Mobile & Desktop Screenshots/Recordings (if there are UI changes)