Skip to content

Improve test connection#4695

Merged
keon94 merged 6 commits intoapache:mainfrom
merico-ai:improve_test_connection
Mar 21, 2023
Merged

Improve test connection#4695
keon94 merged 6 commits intoapache:mainfrom
merico-ai:improve_test_connection

Conversation

@CamilleTeruel
Copy link
Copy Markdown
Contributor

@CamilleTeruel CamilleTeruel commented Mar 16, 2023

Summary

  • Make connection.name optional when calling test-connection
  • Include error messages in response body
  • Rename connection's pat to token
  • Allow empty connection's proxy

@mintsweet mintsweet requested a review from klesh March 17, 2023 01:28
@CamilleTeruel CamilleTeruel force-pushed the improve_test_connection branch 3 times, most recently from b2e3043 to 20aa816 Compare March 20, 2023 09:45
def remote_scope_groups(self, ctx) -> list[RemoteScopeGroup]:
api = AzureDevOpsAPI(ctx.connection)
member_id = api.my_profile.json['id']
def remote_scope_groups(self, connection) -> list[RemoteScopeGroup]:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add type hints


func (pa *pluginAPI) TestConnection(input *plugin.ApiResourceInput) (*plugin.ApiResourceOutput, errors.Error) {
err := pa.invoker.Call("test-connection", bridge.DefaultContext, input.Body).Err
if err != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't assume the error will always be 401. What if the endpoint was offline or gave us another error code? I think TestConnection needs to have a contract where it writes the datasource's response into a standardized output with a Http Code on it. It might as well be the ApiResourceOutput struct, but written for Python.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, this would require to discriminate between errors. We can plan this change for later.
For now the message helps disambiguate.

@CamilleTeruel CamilleTeruel force-pushed the improve_test_connection branch from 406a332 to ef46f06 Compare March 20, 2023 22:05
@keon94
Copy link
Copy Markdown
Contributor

keon94 commented Mar 21, 2023

LGTM

Camille Teruel added 6 commits March 20, 2023 22:54
Since we are not saving the connection at that time there is no need to enforce that DB constrain.
More consistent with other plugins.
Allow to supply an empty str as proxy in connections.
More constistent with other plugins.
@keon94 keon94 force-pushed the improve_test_connection branch from ef46f06 to 254ef6c Compare March 21, 2023 03:54
@keon94 keon94 merged commit d0306be into apache:main Mar 21, 2023
@keon94 keon94 deleted the improve_test_connection branch March 21, 2023 04:04
@keon94 keon94 added this to the v0.17.0 milestone Mar 29, 2023
@keon94 keon94 added the component/plugins This issue or PR relates to plugins label Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/plugins This issue or PR relates to plugins

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants