feat(ui): Added Last run status and error messages to check, tasks, and notification rules #16325

asalem1 · 2019-12-23T23:08:50Z

Problem

check rules and notification rules would not receive relevant data from the API. The UI wasn't built out to display the data. Using the client library vs the generatedRoutes results in a lot of TS issues

Solution

Refactored a good chunk of the client library usage related to the ticket to utilize the generatedRoutes to resolve TS issues. Updated the UI to display newly returned data with the help of @alexpaxton

CHANGELOG.md updated with a link to the PR (not the Issue)

http/notification_rule.go

ebb-tide

This PR makes a lot more functional changes than is expressed in the PR description. Every place we replace @influxdata/influx with src/client is a place where we are changing the behavior of the app. I would like for us to make these changes for each resource one by one in separate PRs, rather than as part of making Label types match. The @influxdata/influx client adds default properties to labels so that the UI can rely on there existing a color and description property for each label by applying defaults if they don't exist even though they are not guaranteed by the API. oats returns values as they are provided by the API. I am not certain how we should go about replacing this functionality. One option is to add defaults to labels every time we fetch a resource? In this PR we are going in the direction of removing label defaults from resources that used to have them, rather than adding defaults to labels that don't currently have them and I'm not sure this is the direction to go.

Also, I think it's a bad idea for us to make API calls relating to a particular resource through more than one client. Clients have slightly different expectations/behaviors, and getting from one and posting with another is really difficult to reason about.

ebb-tide · 2019-12-26T11:10:46Z

http/check_test.go

-	"status": "active"
+	"status": "active",
+	"latestCompleted": "0001-01-01T00:00:00Z",
+	"latestScheduled": "0001-01-01T00:00:00Z"


I'm not sure- but would this not also include LastRunStatus and LastRunError ?

I completely agree in that we need to test out the LastRunStatus and LastRunError. They should be included for tests that result in an error

ui/src/alerting/components/CheckCard.tsx

ebb-tide · 2019-12-26T11:39:55Z

ui/src/alerting/components/CheckCard.tsx

-import {Check, Label, AppState} from 'src/types'
-
+import {Check, AppState} from 'src/types'
+import {Label, Labels} from 'src/client'


Labels are going to be such a mess as we switch from @influxdata/influx to oats!

ui/src/alerting/components/notifications/RuleCard.tsx

ui/src/dashboards/components/DashboardExportOverlay.tsx

ebb-tide · 2019-12-26T12:01:36Z

ui/src/dataLoaders/actions/dataLoaders.ts

-      name: telegrafConfigName,
-      description: telegrafConfigDescription,
-      plugins,
+    const resp = await api.putTelegraf({


Are you comparing this function from oats to the one you are replacing from the other client as you go and making sure they are entirely equivalent? It is making me really nervous that we are essentially rewriting the behavior of the app with these changes just to get label types to match. I would strongly prefer if we made these switches from one client to another each in a separate PR instead of one mega PR.

Hey @ebb-tide , great question since the putTelegraf was the only one that's been a question in my mind with respect to this refactor. I did use the generateRoutes file in conjunction with the old apis to ensure that the endpoints, data, etc... were all the same:

https://raw.githubusercontent.com/influxdata/influxdb-client-js/master/src/api/api.ts

As I mentioned earlier, this putTelegraf one was (and still is) a question mark since we currently don't require the addition of plugins for putTelegraf as part of the parameters. I'd love to discuss this if you're around today

ui/src/tasks/actions/index.ts

ebb-tide · 2019-12-26T12:32:08Z

ui/src/tasks/actions/index.ts

@@ -296,7 +312,7 @@ export const updateTaskStatus = (task: Task) => async dispatch => {

 export const updateTaskName = (task: Task) => async dispatch => {
  try {
-    await client.tasks.update(task.id, task)
+    await api.patchTask({taskID: task.id, data: task})


These two functions are not equivalent. client.tasks.update has:

if (!!updates.cron) { delete original.every } if (!!updates.every) { delete original.cron }

which we would have to do in the UI code if we are switching from one client to the other

Great point, let me know if you're around to talk strategy on it

ui/src/telegrafs/actions/index.ts

asalem1 · 2019-12-26T14:42:41Z

This PR makes a lot more functional changes than is expressed in the PR description. Every place we replace @influxdata/influx with src/client is a place where we are changing the behavior of the app. I would like for us to make these changes for each resource one by one in separate PRs, rather than as part of making Label types match. The @influxdata/influx client adds default properties to labels so that the UI can rely on there existing a color and description property for each label by applying defaults if they don't exist even though they are not guaranteed by the API. oats returns values as they are provided by the API. I am not certain how we should go about replacing this functionality. One option is to add defaults to labels every time we fetch a resource? In this PR we are going in the direction of removing label defaults from resources that used to have them, rather than adding defaults to labels that don't currently have them and I'm not sure this is the direction to go.

Also, I think it's a bad idea for us to make API calls relating to a particular resource through more than one client. Clients have slightly different expectations/behaviors, and getting from one and posting with another is really difficult to reason about.

I totally hear where you're coming from and would agree with you in that this should've been compartmentalized. The issue kind of transformed itself into what it was based on resolving linter errors, hence the reason for its crazy scope. Once Tasks were transformed to update the type, there was a cascade of linter errors that had to be addressed that ran me down from tasks -> labels -> templates -> variables

…sue and update API tests to account for lastRunStatus and lastRunError

drdelambre · 2019-12-27T22:15:52Z

ui/src/alerting/actions/checks.ts

  PostCheck,
 } from 'src/types'
+import {Label} from 'src/client'


if you're for serious about this, it'd be easier to just import the Label definition into 'src/types' instead

drdelambre · 2019-12-27T22:16:29Z

ui/src/alerting/components/endpoints/EndpointCard.tsx

@@ -148,7 +144,7 @@ const EndpointCard: FC<Props> = ({
  }
  const labelsComponent = (
    <InlineLabels
-      selectedLabels={endpoint.labels as Label[]}
+      selectedLabels={endpoint.labels}


drdelambre · 2019-12-27T22:17:25Z

ui/src/dashboards/actions/index.ts

@@ -16,7 +16,7 @@ import {
  updateView as updateViewAJAX,
 } from 'src/dashboards/apis'
 import {createDashboardFromTemplate as createDashboardFromTemplateAJAX} from 'src/templates/api'
-
+import * as api from 'src/client'


be explicit here. it's a contract between modules

but convention

drdelambre · 2019-12-27T22:19:06Z

ui/src/dataLoaders/actions/dataLoaders.ts

@@ -3,13 +3,13 @@ import _ from 'lodash'

 // Apis
 import {client} from 'src/utils/api'
+import * as api from 'src/client'


be explicit

asalem1 requested review from a team December 23, 2019 23:08

asalem1 requested a review from a team as a code owner December 23, 2019 23:08

ghost requested review from drdelambre and ebb-tide and removed request for a team December 23, 2019 23:08

kelwang reviewed Dec 23, 2019

View reviewed changes

http/notification_rule.go Outdated Show resolved Hide resolved

ebb-tide suggested changes Dec 26, 2019

View reviewed changes

asalem1 requested review from ebb-tide and kelwang December 26, 2019 15:38

asalem1 and others added 13 commits December 26, 2019 08:10

feat(ui): added last... to notification rules and checks for API

1c0174e

feat(api): updated tests to account for api changes

d5e119f

feat(ui): added in UI to integrate API changes, need to fix typing is…

b7c41e9

…sue and update API tests to account for lastRunStatus and lastRunError

feat(ui): fixing TS errors

bd34881

refactor: update design of task card to use icon & popover

793a024

refactor: move task card toggle to metadata with label

ed1f403

chore: appease linter

5f71219

feat(ui): need to revisit template typing issues

c535103

fix(ui): resolved non-template linter type errors

72a05b6

feat(ui): completed feature

8afa48f

fix(ui): resolved rebase conflicts

fca72be

fix(ui): addressed failing tests, still need to work on PR changes

a2f70cd

fix(ui): revised dashboards api to resolve broken e2e test

4f13d19

asalem1 force-pushed the last-run-status branch from be3f661 to 4f13d19 Compare December 26, 2019 23:56

drdelambre reviewed Dec 27, 2019

View reviewed changes

asalem1 mentioned this pull request Dec 31, 2019

chore(ui): refactored tasks to use oats rather than client library #16353

Merged

asalem1 closed this Dec 31, 2019

jacobmarble deleted the last-run-status branch January 2, 2024 22:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ui): Added Last run status and error messages to check, tasks, and notification rules #16325

feat(ui): Added Last run status and error messages to check, tasks, and notification rules #16325

asalem1 commented Dec 23, 2019

ebb-tide left a comment

ebb-tide Dec 26, 2019

asalem1 Dec 26, 2019

ebb-tide Dec 26, 2019

ebb-tide Dec 26, 2019

asalem1 Dec 26, 2019

ebb-tide Dec 26, 2019

asalem1 Dec 26, 2019

asalem1 commented Dec 26, 2019

drdelambre Dec 27, 2019

drdelambre Dec 27, 2019

drdelambre Dec 27, 2019

asalem1 Dec 27, 2019

drdelambre Dec 27, 2019

feat(ui): Added Last run status and error messages to check, tasks, and notification rules #16325

feat(ui): Added Last run status and error messages to check, tasks, and notification rules #16325

Conversation

asalem1 commented Dec 23, 2019

Problem

Solution

ebb-tide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asalem1 commented Dec 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment