Skip to content

feat(ui): test notification endpoints #16031

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

Closed
wants to merge 1 commit into from
Closed

Conversation

121watts
Copy link
Contributor

@121watts 121watts commented Nov 22, 2019

Closes #14792

Kapture 2019-11-22 at 11 20 40

@121watts 121watts requested review from a team as code owners November 22, 2019 19:21
return
}

var token *influxdb.Authorization
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks too hacky. @influxdata/compute-team has cleaned this up already for task. We should make sure the query proxy server can handle the context.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kelwang this is what is currently done in the /api/v2/query endpoint. I agree it's hacky and that we need a long term solution, but I'm not sure exactly what that looks like.

@@ -500,6 +637,7 @@ func (h *NotificationEndpointHandler) handlePutNotificationEndpoint(w http.Respo

for _, fld := range edp.SecretFields() {
if fld.Value != nil {
fmt.Printf("hello: %v\n", *fld.Value)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this debug msg

DeleteNotificationEndpoint(ctx context.Context, id ID) (flds []*SecretField, orgID ID, err error)

// TestNotificationEndpoint tests the an endpoints connection to a 3rd party service i.e. Slack, PagerDuty etc.
TestNotificationEndpoint(ctx context.Context, ne NotificationEndpoint, userID ID) error
Copy link
Contributor

Choose a reason for hiding this comment

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

A method prefix Test is like a reserved keyword to run test uniting in golang, thus might confuse people. If you can give an another name, it would be much preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

@desa desa force-pushed the feat/test-endpoints branch 3 times, most recently from 749b2e0 to 2eb1c50 Compare November 25, 2019 21:15
feat(notEnd): add definition to swagger

feat(wip): add endpoint test route

feat(ui): add test endpoint button

feat(wip): execute endpoint test query

feat: implement test endpoint for slack

feat: introduce http endopint test

feat: implement http test endpoint with auth

feat: add test PagerDuty endpoint

fix: PagerDuty test query generation

wip

refactor: implement test endpoint for existing endpoints

feat(endpoint-test): add error reporting

feat(test-endpoint): hide testing on create

fix: errors not caught

test: remove only

test: remove check for test button

fix: thunk typings

test: add testChannel to slack

test: fix go and e2e

fix: test and linting

test: fix comment test

test: fix test

fix(notification/endpoint): add test channel to slack endpoint

Co-Authored-By: Andrew Watkins <[email protected]>
@desa desa force-pushed the feat/test-endpoints branch from 2eb1c50 to ff9fe6c Compare November 25, 2019 21:23
saveButtonText: string
onSave: (endpoint: NotificationEndpoint) => void
onSave: (endpoint: NotificationEndpoint) => Promise<void>
onTest: (endpoint: NotificationEndpoint) => Promise<void>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why we've passed onTest all the way down here without using it?


const resp = await api.postNotificationEndpointsTest({data})

if (resp.status !== 204) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't seem like testEndpoint ends being used in some instances, would it be more beneficial to dispatch a notification to the user?

style={{width: 'auto', marginTop: '8px'}}
>
{errorMessage}
{message.text}
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the text is undefined? Should we have a generic message to let the user know that something wrong happened?

@jacobmarble jacobmarble closed this Jan 2, 2024
@jacobmarble jacobmarble deleted the feat/test-endpoints branch January 2, 2024 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add endpoint test to endpoint configuration
5 participants