Alex/mvp UI for dbt cloud integration #18095
There are no files selected for viewing
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,113 @@ | ||||||||
| // This module is for the business logic of working with dbt Cloud webhooks. | ||||||||
| // Static config data, urls, functions which wrangle the APIs to manipulate | ||||||||
| // records in ways suited to the UI user workflows--all the implementation | ||||||||
| // details of working with dbtCloud jobs as webhook operations, all goes here. | ||||||||
| // The presentation logic and orchestration in the UI all goes elsewhere. | ||||||||
| // | ||||||||
| // About that business logic: | ||||||||
| // - for now, the code treats "webhook operations" and "dbt Cloud job" as synonymous. | ||||||||
| // - custom domains aren't yet supported | ||||||||
|
|
||||||||
| import isEmpty from "lodash/isEmpty"; | ||||||||
| import { useMutation } from "react-query"; | ||||||||
|
|
||||||||
| import { OperatorType, WebBackendConnectionRead, OperationRead } from "core/request/AirbyteClient"; | ||||||||
| import { useWebConnectionService } from "hooks/services/useConnectionHook"; | ||||||||
| import { useCurrentWorkspace } from "hooks/services/useWorkspace"; | ||||||||
| import { useUpdateWorkspace } from "services/workspaces/WorkspacesService"; | ||||||||
|
|
||||||||
| export interface DbtCloudJob { | ||||||||
| account: string; | ||||||||
| job: string; | ||||||||
| operationId?: string; | ||||||||
| } | ||||||||
| const dbtCloudDomain = "https://cloud.getdbt.com"; | ||||||||
| const webhookConfigName = "dbt cloud"; | ||||||||
| const executionBody = `{"cause": "airbyte"}`; | ||||||||
| const jobName = (t: DbtCloudJob) => `${t.account}/${t.job}`; | ||||||||
|
|
||||||||
| const toDbtCloudJob = (operation: OperationRead): DbtCloudJob => { | ||||||||
| const { operationId } = operation; | ||||||||
| const { executionUrl } = operation.operatorConfiguration.webhook || {}; | ||||||||
|
|
||||||||
| const matches = (executionUrl || "").match(/\/accounts\/([^/]+)\/jobs\/([^]+)\//); | ||||||||
|
|
||||||||
| if (!matches) { | ||||||||
| throw new Error(`Cannot extract dbt cloud job params from executionUrl ${executionUrl}`); | ||||||||
| } else { | ||||||||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||||||||
| const [_fullUrl, account, job] = matches; | ||||||||
|
Comment on lines
+38
to
+39
There was a problem hiding this comment.
Suggested change
There was a problem hiding this comment. ^ We don't assign unused variables any name, but instead skip them. This is happening in a couple of places around this PR, so we should try to remove all of those. There was a problem hiding this comment. Somehow I didn't realize you can do that when destructuring; I like that much better than using throwaway variables and manually shushing the linter. Will do for all instances. There was a problem hiding this comment. 🎨 Other unused variable, that should be skipped in destructuring. |
||||||||
|
|
||||||||
| return { | ||||||||
| account, | ||||||||
| job, | ||||||||
| operationId, | ||||||||
| }; | ||||||||
| } | ||||||||
| }; | ||||||||
| const isDbtCloudJob = (operation: OperationRead): boolean => | ||||||||
| operation.operatorConfiguration.operatorType === OperatorType.webhook; | ||||||||
|
|
||||||||
| export const useSubmitDbtCloudIntegrationConfig = () => { | ||||||||
| const { workspaceId } = useCurrentWorkspace(); | ||||||||
| const { mutateAsync: updateWorkspace } = useUpdateWorkspace(); | ||||||||
|
|
||||||||
| return useMutation(async (authToken: string) => { | ||||||||
| await updateWorkspace({ | ||||||||
| workspaceId, | ||||||||
| webhookConfigs: [ | ||||||||
| { | ||||||||
| name: webhookConfigName, | ||||||||
| authToken, | ||||||||
| }, | ||||||||
| ], | ||||||||
| }); | ||||||||
| }); | ||||||||
| }; | ||||||||
|
|
||||||||
| export const useDbtIntegration = (connection: WebBackendConnectionRead) => { | ||||||||
| const workspace = useCurrentWorkspace(); | ||||||||
| const { workspaceId } = workspace; | ||||||||
| const connectionService = useWebConnectionService(); | ||||||||
|
|
||||||||
| // TODO extract shared isDbtWebhookConfig predicate | ||||||||
| const hasDbtIntegration = !isEmpty(workspace.webhookConfigs?.filter((config) => /dbt/.test(config.name || ""))); | ||||||||
|
There was a problem hiding this comment. 🎨 This looks like it could just be an There was a problem hiding this comment. I got paranoid about locking in our ability to edit the webhook name if we decide to tweak the naming convention, but I think you're right that it would be cleaner just checking for the actual value, since it's not user-supplied There was a problem hiding this comment. I'm not sure whether we meant the same thing, so just for clarification, I suggested just replacing the regexp by an includes not the whole filter statement, i.e.:
Suggested change
|
||||||||
| const webhookConfigId = workspace.webhookConfigs?.find((config) => /dbt/.test(config.name || ""))?.id; | ||||||||
|
|
||||||||
| const dbtCloudJobs = [...(connection.operations?.filter((operation) => isDbtCloudJob(operation)) || [])].map( | ||||||||
| toDbtCloudJob | ||||||||
| ); | ||||||||
| const otherOperations = [...(connection.operations?.filter((operation) => !isDbtCloudJob(operation)) || [])]; | ||||||||
|
|
||||||||
| const saveJobs = (jobs: DbtCloudJob[]) => { | ||||||||
| // TODO dynamically use the workspace's configured dbt cloud domain when it gets returned by backend | ||||||||
| const urlForJob = (job: DbtCloudJob) => `${dbtCloudDomain}/api/v2/accounts/${job.account}/jobs/${job.job}/run`; | ||||||||
|
|
||||||||
| return connectionService.update({ | ||||||||
| connectionId: connection.connectionId, | ||||||||
| operations: [ | ||||||||
| ...otherOperations, | ||||||||
| ...jobs.map((job) => ({ | ||||||||
| workspaceId, | ||||||||
| ...(job.operationId ? { operationId: job.operationId } : {}), | ||||||||
| name: jobName(job), | ||||||||
| operatorConfiguration: { | ||||||||
| operatorType: OperatorType.webhook, | ||||||||
| webhook: { | ||||||||
| executionUrl: urlForJob(job), | ||||||||
| // if `hasDbtIntegration` is true, webhookConfigId is guaranteed to exist | ||||||||
| ...(webhookConfigId ? { webhookConfigId } : {}), | ||||||||
| executionBody, | ||||||||
| }, | ||||||||
| }, | ||||||||
| })), | ||||||||
| ], | ||||||||
| }); | ||||||||
| }; | ||||||||
|
|
||||||||
| return { | ||||||||
| hasDbtIntegration, | ||||||||
| dbtCloudJobs, | ||||||||
| saveJobs, | ||||||||
| }; | ||||||||
| }; | ||||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| @use "scss/colors"; | ||
| @use "scss/variables" as vars; | ||
|
|
||
| $item-spacing: 25px; | ||
|
There was a problem hiding this comment. Would be great if we can use the spacing variables in |
||
|
|
||
| .controlGroup { | ||
| display: flex; | ||
| justify-content: flex-end; | ||
| margin-top: $item-spacing; | ||
|
|
||
| .button { | ||
| margin-left: 1em; | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| import { Field, FieldProps, Form, Formik } from "formik"; | ||
| import React from "react"; | ||
| import { FormattedMessage } from "react-intl"; | ||
|
|
||
| import { LabeledInput } from "components/LabeledInput"; | ||
| import { Button } from "components/ui/Button"; | ||
|
|
||
| import { useSubmitDbtCloudIntegrationConfig } from "packages/cloud/services/dbtCloud"; | ||
| import { Content, SettingsCard } from "pages/SettingsPage/pages/SettingsComponents"; | ||
|
|
||
| import styles from "./DbtCloudSettingsView.module.scss"; | ||
|
|
||
| export const DbtCloudSettingsView: React.FC = () => { | ||
| const { mutate: submitDbtCloudIntegrationConfig, isLoading } = useSubmitDbtCloudIntegrationConfig(); | ||
| return ( | ||
| <SettingsCard title={<FormattedMessage id="settings.integrationSettings.dbtCloudSettings" />}> | ||
| <Content> | ||
| <Formik | ||
| initialValues={{ | ||
| serviceToken: "", | ||
| }} | ||
| onSubmit={({ serviceToken }) => submitDbtCloudIntegrationConfig(serviceToken)} | ||
| > | ||
| <Form> | ||
| <Field name="serviceToken"> | ||
| {({ field }: FieldProps<string>) => ( | ||
| <LabeledInput | ||
| {...field} | ||
| label={<FormattedMessage id="settings.integrationSettings.dbtCloudSettings.form.serviceToken" />} | ||
| type="text" | ||
| /> | ||
| )} | ||
| </Field> | ||
| <div className={styles.controlGroup}> | ||
| <Button variant="primary" type="submit" className={styles.button} isLoading={isLoading}> | ||
| <FormattedMessage id="settings.integrationSettings.dbtCloudSettings.form.submit" /> | ||
| </Button> | ||
| </div> | ||
| </Form> | ||
| </Formik> | ||
| </Content> | ||
| </SettingsCard> | ||
| ); | ||
| }; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| @use "scss/colors"; | ||
|
|
||
| .jobListContainer { | ||
| padding: 25px 25px 22px; | ||
| background-color: colors.$grey-50; | ||
| } | ||
|
|
||
| .jobListTitle { | ||
| display: flex; | ||
| justify-content: space-between; | ||
| } | ||
|
|
||
| .jobListForm { | ||
| width: 100%; | ||
| } | ||
|
|
||
| .emptyListContent { | ||
| display: flex; | ||
| flex-direction: column; | ||
| align-items: center; | ||
|
|
||
| > img { | ||
|
There was a problem hiding this comment. 🎨 We have that There was a problem hiding this comment. Agreed, this was 100% a time-saving hack; I'm not a fan of coupling style and html structure at all. |
||
| width: 111px; | ||
| height: 111px; | ||
| margin: 20px 0; | ||
|
|
||
| } | ||
| } | ||
|
|
||
| .contextExplanation { | ||
| color: colors.$grey-300; | ||
| width: 100%; | ||
|
|
||
| & a { | ||
| color: colors.$grey-300; | ||
| } | ||
| } | ||
|
|
||
| .jobListButtonGroup { | ||
| display: flex; | ||
| justify-content: flex-end; | ||
| margin-top: 20px; | ||
|
|
||
| width: 100%; | ||
| } | ||
|
|
||
| .jobListButton { | ||
| margin-left: 10px; | ||
| } | ||
|
|
||
| .jobListItem { | ||
| margin-top: 10px; | ||
| padding: 18px; | ||
| width: 100%; | ||
| display: flex; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
|
|
||
| & img { | ||
|
There was a problem hiding this comment. Same as above, we should give this |
||
| height: 32px; | ||
| width: 32px; | ||
| } | ||
| } | ||
|
|
||
| .jobListItemIntegrationName { | ||
| display: flex; | ||
| align-items: center; | ||
| } | ||
|
|
||
| .jobListItemInputGroup { | ||
| display: flex; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
| } | ||
|
|
||
| .jobListItemInput { | ||
| height: fit-content; | ||
| margin-left: 1em; | ||
| } | ||
|
|
||
| .jobListItemInputLabel { | ||
| font-size: 11px; | ||
| font-weight: 500; | ||
| } | ||
|
|
||
| .jobListItemDelete { | ||
| color: colors.$grey-200; | ||
| font-size: large; | ||
| margin: 0 1em; | ||
| cursor: pointer; | ||
| border: none; | ||
| background-color: inherit; | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my understanding, does it mean the job actually only stores the full execution URL, while we're entering in the UI individual separate fields?
I think the backend should store the fields we want the user to actually enter, and just build the executionUrl when doing the request. Parsing this client side sounds like a fragile method, and if we don't store this data now, we might have a problem addressing this in a backwards compatible way easily, since we already lost the individual field information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good callout! We did discuss this a bit.
The backend is actually unaware of dbt accounts/jobs, or dbt cloud at all - it's a "webhook operation", and it just holds the plain URL. dbt Cloud is a totally frontend concept at the moment.
With that said, I do think a better approach would be what you're describing, and the way I'd do it would be to accept the URL "template" that includes named params; plus a map of the params. e.g.,
"https://cloud.getdbt.com/api/v2/accounts/{accountId}/jobs/{jobId}/run/", {"accountId": "x", "jobId": "y"}.I think for existing configs we would represent this like
"https://cloud.getdbt.com/api/v2/accounts/x/jobs/y/run/", {}. Since dbt Cloud is going to be the only "webhook operation" integration in the UI for the foreseeable future, once we have thetemplate, paramsrepresentation we can run a targeted migration since we know what these look like, and then we'd be able to kill this bit of messy FE code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davinchia any thoughts here? I do agree we should make the change in the backend ~asap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I 100% agree; I think there should be cloud endpoints which are specific to dbt Cloud, return the easy-to-use
DbtCloudJobobjects, and, down the line, support UI niceties like selecting dbt jobs from a dropdown (which has to go through the backend for CORS reasons) and the fact that these are backed by a generic webhook record remaining an implementation detail hidden from the frontend code. (In fact, it's one of the primary reasons I tried to keep all the API interactions contained to this single file.) It came up a bit in earlier discussions and it's something I'm going to advocate for in the immediate clean-up planning/spec work. Thoughts, @mfsiega-airbyte?For what it's worth, this seems like a reasonable PR sequencing of that change if we do choose to ship the MVP with these API contracts still in place:
airbyte-cloudadding new dbt-Cloud-specific APIs (which wouldn't yet be used);airbyteto define and generate those new APIs and replace all the webhook-oriented code in this file with them.That said, in terms of this PR, I didn't have a working instance of this more minimal backend implementation to develop and experiment with until this weekend, so I had little choice but to run with what was available to get something working for the demo.