-
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] Add logic for query params #160515
[Console] Add logic for query params #160515
Conversation
…-ref HEAD~1..HEAD --fix'
*/ | ||
|
||
/** | ||
* --------------- THIS FILE IS COPIED FROM ES SPECIFICATION REPO ------------------- |
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 added a task to the meta issue to see if we could import the types instead of copying
Pinging @elastic/platform-deployment-management (Team:Deployment Management) |
The current UI for url params is limited so I open this issue to track some improvements. |
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.
Nice one @yuliacech! I tested locally following the instructions and everything seems to be working as expected, I checked the changes in the generated file and seems to be correct.
I was wondering though if it would make sense to include the testing commands into the package readme along with some more deets about how this works so that whoever needs to run it or pick this work up has more context (this could totally be done with a separate issue though!)
* - "__flag__" for a boolean (suggesting value 'true' and 'false') | ||
* - list of options in an array, for example ['30s', '-1', '0'], suggesting all 3 values in a list | ||
* If there is only a default value, we need to wrap it in an array, so that this value is displayed in a suggestion (similar to the list). | ||
* Numbers need to be converted to strings, otherwise they are not displayed as suggestions. |
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.
interesting! is this a bug from the editor? 🤔
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 haven't investigated in too much detail, but I added a note about numbers to the issue I opened for the current UI for url params. Basically, autocomplete suggestions currently only work for booleans and lists and could be improved.
return booleanFlagString; | ||
} else { | ||
// if default value, convert to string and put in an array | ||
return serverDefault ? [serverDefault + ''] : ''; |
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.
nit: serverDefault.toString()
|
||
const convertLiteralValue = (type: SpecificationTypes.LiteralValue): UrlParamValue | undefined => { | ||
// convert the value to a string | ||
return [type.value + '']; |
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.
nit: type.value.toString()
Thanks a lot for your review, @sabarasaba! Great idea about the readme file, I updated it with the instructions how to run the command as you suggested. |
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
Fixes #160528
Follow up to #159241
This PR adds logic for query parameters to the new script generating Console autocomplete definitions from ES specification. The logic is explained in details in the file but here is a short version:
__flag__
) and a list of options (for example['all', 'open', 'hidden']
). The script will convert all of the url params from the specification into this format: a boolean or a list. If there are no set options for a parameter, but a default value is known, then it will be converted into a list with 1 option, for example['random']
so that the autocomplete engine will display it as a single suggestion. We also need to convert any numbers to strings, because they won't be displayed otherwise.request
which in turn has 2 properties describing url params:attachedBehaviours
andquery
. Both object contain an array ofproperty
's each describing a url param.property
is configured with either a built in type (string
,number
,boolean
) or defined type, for exampleExpandWildcards
. By finding the typeExpandWildcards
in the specification, we can convert this type to a list of options['open', 'all', 'none', 'hidden']
.How to test
Similar to #159241, you need to re-generenate the definitions and see if local changes to definitions files make sense.
node scripts/generate_console_definitions.js --source <ES_SPECIFICATION_REPO> --emptyDest
KIBANA_REPO/src/plugins/console/server/lib/spec_definitions/json/generated
Intended changes to the definitions files
CommonQueryParameters
in the specification (see this file).The converted url params
url_components
property in the definitions is deleted and this change will be addressed separately. (not sure it is currently working but added a task to the meta issue)0
or0.0
but that is not currently displayed in suggestions. Instead, the new script converts numbers to strings and if any default value is present, it will be displayed as a suggestion.