Conversation
…ow auth redirects, use example.com in address placeholder, downgrade missing-database datasource notice to warning
|
| placeholder="${serverType === ServerType.KDB | ||
| ? "127.0.0.1 or localhost" | ||
| : `myinsights.clouddeploy.com`}" | ||
| : `https://myinsights.example.com`}" |
There was a problem hiding this comment.
After manually testing the connection dialog for a while, there are some changes that could make the field names and descriptions clearer.
For Server Name and Name the server., it's not obvious that this can be any name you like. I can see a user asking "How do I know what my server name is?". It might make more sense if the title were "Connection Name" and the description was "A name to uniquely identify this connection".
And for the description of "Define connection address" the current description for non-Insights connections is Set the IP of your kdb+ database connection, your Insights connection must be deployed for kdb VS Code to access., but there are a few issues with that
- IP addresses will probably be less common that domain names (as many certificate authorities won't issue certificates for bare IP addresses). Given that IP addresses need to be prefixed with the protocol anyways, it seems simpler to just say URL
- Insights has many different IP addresses/URLs for the different database microservices, so "kdb+ database connection" is ambiguous here. "Insights Instance" would be clearer
- It seems odd to say that your Insights connection must be deployed in the connection creation dialog. Was this meant to say "Insights Instance?"
For Insights connections, I'd propose the description "The URL for your Insights instance. This instance must be deployed for the extension to access it."
and for non-Insights connections, I'd propose "The IP or URL of your kdb+ process"
There was a problem hiding this comment.
@bjeffery-kx This may need another ticket. This is for https://kxl.atlassian.net/browse/KXI-72058 and I need this to be merged in my connection refactoring.
2 participants
Changes introduced by this PR
https://for Insights connection URLs — reject protocol-less andhttp://URLs with a validation error (add & edit).getAuthPrefix(maxRedirects0 → 5) instead of misreading a 301 as needing theauth/prefix.https://myinsights.example.com.http://, protocol-less, and empty URLs.