Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add appropriate workflow count to our connected notification cards and details page #1302

Merged
merged 5 commits into from
May 11, 2023
Merged

Add appropriate workflow count to our connected notification cards and details page #1302

merged 5 commits into from
May 11, 2023

Conversation

kenxu95
Copy link
Contributor

@kenxu95 kenxu95 commented May 9, 2023 •
edited
Loading

@likawind for backend review. @agiron123 to look at frontend.

Describe your changes and why you are making these changes

See the demo. This adds the appropriate workflow counts and frontend alterations to the connected email and slack resources.

Related issue number (if any)

ENG-2893
Also fixes ENG-2949

Loom demo (if any)

https://www.loom.com/share/5a96f8a72146441ea18eadac8ec08306

Checklist before requesting a review

  • I have created a descriptive PR title. The PR title should complete the sentence "This PR...".
  • I have performed a self-review of my code.
  • I have included a small demo of the changes. For the UI, this would be a screenshot or a Loom video.
  • If this is a new feature, I have added unit tests and integration tests.
  • I have run the integration tests locally and they are passing.
  • I have run the linter script locally (See python3 scripts/run_linters.py -h for usage).
  • All features on the UI continue to work correctly.
  • Added one of the following CI labels:
    • run_integration_test: Runs integration tests
    • skip_integration_test: Skips integration tests (Should be used when changes are ONLY documentation/UI)

@kenxu95 kenxu95 requested review from agiron123 and likawind May 9, 2023 20:00
kenxu95
kenxu95 commented May 9, 2023
View reviewed changes
src/golang/lib/workflow/operator/service.go
"github.com/aqueducthq/aqueduct/lib/models"
"github.com/aqueducthq/aqueduct/lib/models/shared"
"github.com/aqueducthq/aqueduct/lib/repos"
"github.com/dropbox/godropbox/errors"
"github.com/google/uuid"
)

// GetOperatorsOnIntegraiton will return an empty list for notification resources.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@likawind so instead of fetching the operators for notification resources, which would lead to unnecessary performance cost, I just had callers that cared about notifications use GetWorkflowIDsUsingNotification() instead. Callers that don't care about returning no operators for notifications will just get an empty list. This meant a couple extra checks in the frontend, but it seemed to be the fastest way forward without incurring too much debt.

src/ui/common/src/components/integrations/dialogs/dialog.tsx
const numWorkflows = new Set(operators.map((x) => x.workflow_id)).size;
const numWorkflows = operators
? new Set(operators.map((x) => x.workflow_id)).size
: 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@agiron123 this is a small change to the dialog component that stopped email resources from crashing on any edits or deletes, which was a pain when developing. I don't think it should mess up what you've been doing.

likawind
likawind approved these changes May 9, 2023
View reviewed changes
Copy link
Contributor

@likawind likawind left a comment

Choose a reason for hiding this comment

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

Backend LGTM. I also don't see strong antipattern in frontend

src/golang/lib/lib_utils/utils.go Outdated
@@ -213,6 +214,32 @@ func ParseEmailConfig(conf auth.Config) (*shared.EmailConfig, error) {
}, nil
}

const (
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel this lib_utils library is really badly named. We should remove stuffs out and avoid adding new stuffs if possible. Is there a better existing library (maybe /notification) we can put this?

Copy link
Contributor Author

@kenxu95 kenxu95 May 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

hmm doing so seems to induce an import cycle, I think /notification is at too high of a layer right now. I've moved it into a separate file in lib_utils called notification.go for now. Agreed this name could be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm yeah let's keep it in this lib for now

src/golang/lib/models/shared/service.go
@@ -157,6 +157,10 @@ func IsComputeIntegration(service Service) bool {
return ok
}

func IsNotificationResource(service Service) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you intend to rename other IsXIntegration in near future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

likawind
likawind approved these changes May 9, 2023
View reviewed changes
Copy link
Contributor

@likawind likawind left a comment

Choose a reason for hiding this comment

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

Backend LGTM. I also don't see strong antipattern in frontend

@kenxu95 kenxu95 added the run_integration_test Triggers integration tests label May 9, 2023
@kenxu95 kenxu95 force-pushed the eng-2893-notification-s3-data-integration branch from ef4b6a2 to 670a0a3 Compare May 11, 2023 16:05
@kenxu95 kenxu95 force-pushed the eng-2893-notification-s3-data-integration branch from 670a0a3 to d786fd7 Compare May 11, 2023 18:23
@kenxu95 kenxu95 merged commit 86269ec into main May 11, 2023
@kenxu95 kenxu95 deleted the eng-2893-notification-s3-data-integration branch May 11, 2023 19:09
likawind pushed a commit that referenced this pull request May 12, 2023
@kenxu95 @likawind
Add appropriate workflow count to our connected notification cards an…
1969288 
…d details page (#1302)
likawind added a commit that referenced this pull request May 17, 2023
@likawind
Squashed commit of the following:
4196cf7 
commit e1fd9d7
Merge: 370665e 2fad420
Author: Wei Chen <wei@aqueducthq.com>
Date:   Wed May 17 14:19:11 2023 -0700

    Merge branch 'eng-2699-m1-refactor-wf-detail-pages-to-use-new' into eng-2699-m1-refactor-wf-detail-pages-to-use-new-2

commit 2fad420
Merge: b4485f4 676e81a
Author: Wei Chen <wei@aqueducthq.com>
Date:   Wed May 17 14:06:26 2023 -0700

    Merge branch 'eng-2699-m1-refactor-wf-detail-pages-to-use-new' of https://github.com/aqueducthq/aqueduct into eng-2699-m1-refactor-wf-detail-pages-to-use-new

commit b4485f4
Author: Wei Chen <wei@aqueducthq.com>
Date:   Mon Apr 24 15:43:51 2023 -0700

    rebase

commit d3a2669
Merge: e60ac09 18011cb
Author: Wei Chen <wei@aqueducthq.com>
Date:   Wed May 17 14:04:32 2023 -0700

    Merge branch 'eng-2712-m1-migrate-wf-edit-trigger-endpoints-to' into eng-2943-investigate-result-content-route-tests

commit 18011cb
Merge: ec515cf 8cb356c
Author: Wei Chen <wei@aqueducthq.com>
Date:   Wed May 17 14:02:47 2023 -0700

    Merge branch 'eng-2847-m1-implement-wfiddags-endpoint-with' into eng-2712-m1-migrate-wf-edit-trigger-endpoints-to

commit 8cb356c
Merge: ec94379 b3946a4
Author: Wei Chen <wei@aqueducthq.com>
Date:   Wed May 17 14:02:23 2023 -0700

    Merge branch 'main' of https://github.com/aqueducthq/aqueduct into eng-2847-m1-implement-wfiddags-endpoint-with

commit 676e81a
Author: Wei Chen <wei@aqueducthq.com>
Date:   Mon Apr 24 15:43:51 2023 -0700

    rebase

commit ec94379
Author: Wei Chen <wei@aqueducthq.com>
Date:   Wed May 17 13:28:38 2023 -0700

    comments

commit b3946a4
Author: kenxu95 <kenxu@aqueducthq.com>
Date:   Wed May 17 10:19:11 2023 -0700

    Move any overflow rows on the details header fields into an additional column (#1322)

commit e60ac09
Author: Wei Chen <wei@aqueducthq.com>
Date:   Tue May 16 20:04:49 2023 -0700

    fix

commit ec515cf
Merge: 1d0482a f81c065
Author: Wei Chen <wei@aqueducthq.com>
Date:   Tue May 16 17:37:04 2023 -0700

    Merge branch 'eng-2847-m1-implement-wfiddags-endpoint-with' into eng-2712-m1-migrate-wf-edit-trigger-endpoints-to

commit f81c065
Author: Wei Chen <wei@aqueducthq.com>
Date:   Tue May 16 17:34:45 2023 -0700

    lint

commit 1d0482a
Author: Wei Chen <wei@aqueducthq.com>
Date:   Tue May 16 17:29:44 2023 -0700

    works

commit 5ad5130
Author: Andre Giron <andre@aqueducthq.com>
Date:   Tue May 16 16:34:41 2023 -0700

    ENG-2979: Fixes bigquery and gcs dialogs that were crashing after file uploads. (#1326)

    Co-authored-by: Andre Giron <andre@spiralai.co>

commit 03aa76e
Author: Andre Giron <andre@aqueducthq.com>
Date:   Tue May 16 16:33:35 2023 -0700

    ENG-2979 S3 Dialog Validation Fixes (#1328)

    Co-authored-by: Andre Giron <andre@spiralai.co>

commit f63c5f3
Author: kenxu95 <kenxu@aqueducthq.com>
Date:   Tue May 16 16:24:14 2023 -0700

    Fix Snowflake save missing schema bug (#1331)

commit a458ce6
Author: Wei Chen <wei@aqueducthq.com>
Date:   Tue May 16 15:27:52 2023 -0700

    works

commit f529c5f
Author: Saurav Chhatrapati <saurav@aqueducthq.com>
Date:   Tue May 16 13:36:12 2023 -0400

    Fixes cloudpickle serialization for Python 3.7 (#1324)

commit c335fd5
Author: Saurav Chhatrapati <saurav@aqueducthq.com>
Date:   Tue May 16 12:15:48 2023 -0400

    Adds error checking for whether big query dataset exists (#1319)

commit d3521a4
Author: kenxu95 <kenxu@aqueducthq.com>
Date:   Mon May 15 20:58:16 2023 -0700

    Add ability to parameterize the save operator (#1320)

commit 942dec1
Author: kenxu95 <kenxu@aqueducthq.com>
Date:   Mon May 15 15:36:21 2023 -0700

    [UI] Merge the Conda resource into the Aqueduct Server  (#1311)

commit ccf4e17
Author: Wei Chen <wei@aqueducthq.com>
Date:   Mon May 15 14:57:19 2023 -0700

    [2/n] Adds gh actions to publish test pypi packages (#1262)

commit fb67044
Author: kenxu95 <kenxu@aqueducthq.com>
Date:   Mon May 15 12:04:48 2023 -0700

    Fix bug where duplicate fields showing in resource details dropdown (#1321)

commit cfeeb2f
Author: kenxu95 <kenxu@aqueducthq.com>
Date:   Mon May 15 11:45:31 2023 -0700

    Fix generated docstrings when viewing resource methods in Jupyter (#1315)

commit b544252
Author: eunice-chan <30596854+eunice-chan@users.noreply.github.com>
Date:   Fri May 12 21:26:55 2023 -0500

    Eng 2884 test node routes (#1299)

    Co-authored-by: Eunice Chan <eunice@aqueducthq.com>
    Co-authored-by: Wei Chen <wei@aqueducthq.com>

commit a8abb16
Author: Andre Giron <andre@aqueducthq.com>
Date:   Fri May 12 16:58:59 2023 -0700

    Fixes broken storybook build (#1317)

    Co-authored-by: Andre Giron <andre@spiralai.co>

commit d6ed5ca
Author: kenxu95 <kenxu@aqueducthq.com>
Date:   Fri May 12 10:56:42 2023 -0700

    Update the workflow "No Run" logo (#1312)

commit c9fec70
Author: Chenggang Wu <cgwu0530@gmail.com>
Date:   Fri May 12 09:30:54 2023 -0700

    README typo fix (#1313)

commit b686014
Author: Andre Giron <andre@aqueducthq.com>
Date:   Thu May 11 12:49:53 2023 -0700

    ENG-2767 react hook form submission (#1232)

    Co-authored-by: Andre Giron <andre@spiralai.co>
    Co-authored-by: Chenggang Wu <cgwu0530@gmail.com>
    Co-authored-by: kenxu95 <kenxu@aqueducthq.com>

commit 86269ec
Author: kenxu95 <kenxu@aqueducthq.com>
Date:   Thu May 11 12:09:42 2023 -0700

    Add appropriate workflow count to our connected notification cards and details page (#1302)

commit 7c75ad6
Author: kenxu95 <kenxu@aqueducthq.com>
Date:   Thu May 11 10:45:52 2023 -0700

    Update the artifact storage card presentation to be consistent with the other data integrations (#1297)

commit 4f9c227
Author: Andre Giron <andre@aqueducthq.com>
Date:   Wed May 10 16:55:23 2023 -0700

    ENG_2960: Fix broken documentation link for Aqueduct demo resource. (#1306)

    Co-authored-by: Andre Giron <andre@spiralai.co>

commit 6341917
Author: Andre Giron <andre@aqueducthq.com>
Date:   Wed May 10 16:28:23 2023 -0700

    Release version 0.3.2 (#1307)

    Co-authored-by: Vikram Sreekanti <vsreekanti@gmail.com>
    Co-authored-by: Andre Giron <andre@spiralai.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@likawind likawind likawind approved these changes

@agiron123 agiron123 Awaiting requested review from agiron123

Assignees
No one assigned
Labels
run_integration_test Triggers integration tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kenxu95 @likawind