[FEATURE] Add test datasource connection button - #99
Conversation
3df35e7 to
6e06d51
Compare
6e06d51 to
bcda582
Compare
| const existing = draft.proxy.spec.allowedEndpoints ?? []; | ||
| const alreadyAllowed = existing.some((e) => e.endpointPattern === healthCheckPath && e.method === 'GET'); | ||
| if (!alreadyAllowed) { | ||
| draft.proxy.spec.allowedEndpoints = [...existing, { endpointPattern: healthCheckPath, method: 'GET' }]; |
There was a problem hiding this comment.
This is inserted as a regex, would it fail it the dots or other reserved characters are not escaped?
| }, [testConnection, successSnackbar, exceptionSnackbar]); | ||
|
|
||
| return ( | ||
| <Button onClick={handleClick} color="info" variant="outlined" {...buttonProps}> |
There was a problem hiding this comment.
We probably should have a testing... state and block the button while is testing, otherwise rapid clicks can cause multiple snackbars.
| const pluginSpec = spec.plugin.spec; | ||
|
|
||
| if (hasDirectUrl(pluginSpec)) { | ||
| await fetch(`${pluginSpec.directUrl}${normalizedPath}`, { |
There was a problem hiding this comment.
should we sanitize this url?, it seems it might contain double slash // if directUrl ends with slash
| value: Spec; | ||
| onChange: (next: Spec) => void; | ||
| isReadonly?: boolean; | ||
| testConnection?: () => Promise<void>; |
There was a problem hiding this comment.
This seems to apply for all plugins, is there a more concrete interface for the datasources editors?
45bba07 to
3dc45bf
Compare
| ); | ||
| } | ||
|
|
||
| function hasHTTPProxy( |
There was a problem hiding this comment.
It seems this function is duplicated in the plugin-system. Should we export it here and re use it?
| const escapedPattern = escapeRegExp(healthCheckPath); | ||
| const augmentedPluginSpec = hasHTTPProxy(value) | ||
| ? produce(value, (draft) => { | ||
| const existing = draft.proxy.spec.allowedEndpoints ?? []; |
There was a problem hiding this comment.
can draft.proxy.spec be undefined? it seems the guard is only checking the proxy.kind
There was a problem hiding this comment.
added additional checks to guard
| testConnection, | ||
| ...others | ||
| }: DatasourceSpecEditorProps): ReactElement | null { | ||
| const healthCheckPath = plugin.healthCheckPath; |
There was a problem hiding this comment.
should we normalize the healthCheckPath before same as is being done when creating the connection function: https://github.com/perses/shared/pull/99/changes#diff-213ce5cb395fb15fbbfba8fc2d90021672b0bc625e22d683f35faf7341cec040R79
There was a problem hiding this comment.
I don't think so, we normalize it there because we concatenate it with first part of url, here we just pass it to test function (the one that normalizes)
| const pluginSpec = spec.plugin.spec; | ||
|
|
||
| if (hasDirectUrl(pluginSpec)) { | ||
| await fetch(`${pluginSpec.directUrl.replace(/\/$/, '')}${normalizedPath}`, { |
There was a problem hiding this comment.
We should check that the normalized path is not empty, otherwise we will be hitting the current UI which might lead to a successful connection check with an empty url
There was a problem hiding this comment.
Changed this code to URL, this way if this is not valid url we will get exception
| it('does not pass testConnection when plugin has no healthCheckPath', async () => { | ||
| const testConnection = jest.fn(); | ||
| renderComponent({ | ||
| pluginSelection: { type: 'Variable', kind: 'ErnieVariable1' }, |
There was a problem hiding this comment.
This test is not testing the correct plugin, in this case is a Variable plugin
There was a problem hiding this comment.
I've added another datasource test plugin without healthcheck path
Signed-off-by: Adrian Sepiół <a.sepiol@sap.com>
…ht to prevent duplicate calls Signed-off-by: Adrian Sepiół <a.sepiol@sap.com>
…h check path Signed-off-by: Adrian Sepiół <a.sepiol@sap.com>
…ceSpecEditor component Signed-off-by: Adrian Sepiół <a.sepiol@sap.com>
…serting into allowedEndpoints endpointPattern Signed-off-by: Adrian Sepiół <a.sepiol@sap.com>
…on validation Signed-off-by: Adrian Sepiół <a.sepiol@sap.com>
3dc45bf to
45c6b62
Compare
…Button for testing datasource connections
Related pull requests (in order):
Related issue:
perses/perses#1542
Description
Adds a Test Connection button to the datasource editor that validates connectivity before saving. The button is wired through the editor component chain (DatasourceEditorForm → PluginEditor → PluginSpecEditor) where the connection logic is bound to the plugin's healthCheckPath and the current project/dashboard context. As part of this, buildProxyUrl is also migrated to @perses-dev/client, replacing the local implementation in perses/ui/app that will be removed in a companion PR.
Screenshots
In perses/plugins#620
Checklist
[<catalog_entry>] <commit message>naming convention using one of thefollowing
catalog_entryvalues:FEATURE,ENHANCEMENT,BUGFIX,BREAKINGCHANGE,DOC,IGNORE.UI Changes
See e2e docs for more details. Common issues include: