Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: improve on-events script logs #3049

Merged
merged 3 commits into from
Nov 25, 2024
Merged

Conversation

TBonnin
Copy link
Collaborator

@TBonnin TBonnin commented Nov 22, 2024

  • each scripts has its own logs operation
  • can filter by Type = Events

Screenshot 2024-11-22 at 16 36 59

https://linear.app/nango/issue/NAN-2219/on-events-script-follow-up

How to tests:

  1. Set on-events scripts in your nango.yaml
        on-events:
            post-connection-creation:
                - setup
            pre-connection-deletion:
                - cleanup

  1. run nango generate and modify the scripts
  2. run nango deploy dev
  3. Go to the dashboard and create/delete a connection for the integration you added the scripts into
  4. Check the logs

@TBonnin TBonnin requested a review from a team November 22, 2024 21:44
Copy link

linear bot commented Nov 22, 2024

@@ -13,7 +13,7 @@ export interface FormatMessageData {
environment?: { id: number; name: string } | undefined;
connection?: { id: number; name: string } | undefined;
integration?: { id: number; name: string; provider: string } | undefined;
syncConfig?: { id: number; name: string } | undefined;
Copy link
Collaborator Author

@TBonnin TBonnin Nov 22, 2024

Choose a reason for hiding this comment

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

syncConfig is already misnamed since it also applies to action. A better name could be simply script.
I attempted to rename it but it has a lot of implications including modifying ES models so I gave up cc @bodinsamuel

Copy link
Collaborator

Choose a reason for hiding this comment

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

it was referencing the table which worked with action but indeed does not scale anymore. We can create a new field yes 👍🏻

@@ -13,7 +13,7 @@ export interface FormatMessageData {
environment?: { id: number; name: string } | undefined;
connection?: { id: number; name: string } | undefined;
integration?: { id: number; name: string; provider: string } | undefined;
syncConfig?: { id: number; name: string } | undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

it was referencing the table which worked with action but indeed does not scale anymore. We can create a new field yes 👍🏻


export interface OperationOnEvents {
type: 'events';
action: 'run';
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not put the actual event name here?

Copy link
Collaborator Author

@TBonnin TBonnin Nov 25, 2024

Choose a reason for hiding this comment

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

at first I though that we wouldn't need to update the Operation when adding new event types but yes we could differentiate based on the event type. fixed in 7fe5d86

@TBonnin TBonnin requested a review from a team November 25, 2024 16:54
Copy link
Contributor

@nalanj nalanj left a comment

Choose a reason for hiding this comment

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

Looks good. For me in the logs these showed up under auth events:

image

@TBonnin TBonnin force-pushed the tbonnin/nan-2219/on-events-logs branch from 7fe5d86 to ca96f79 Compare November 25, 2024 18:59
@TBonnin TBonnin enabled auto-merge (squash) November 25, 2024 18:59
@TBonnin
Copy link
Collaborator Author

TBonnin commented Nov 25, 2024

Looks good. For me in the logs these showed up under auth events:

@nalanj Are you sure you have checkout/built correctly because it should show as EVENTS with the name of the script correctly set

@TBonnin TBonnin merged commit e23709b into master Nov 25, 2024
20 checks passed
@TBonnin TBonnin deleted the tbonnin/nan-2219/on-events-logs branch November 25, 2024 19:07
@nalanj
Copy link
Contributor

nalanj commented Nov 25, 2024

@TBonnin Yeah, something weird was going on there. Got EVENT after another runthrough.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants