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

Sprig Action Destination #2555

Closed
wants to merge 13 commits into from
Closed

Sprig Action Destination #2555

wants to merge 13 commits into from

Conversation

earlsprig
Copy link

This PR creates a new Sprig Action Destination for cloud mode. This integration supports Track, Identify, and Screen calls.

Testing

Tests included for both actions defined in this integration.

  • Added unit tests for new functionality
  • Tested end-to-end using the local server
  • [If destination is already live] Tested for backward compatibility of destination. Note: New required fields are a breaking change.
  • [Segmenters] Tested in the staging environment
  • [Segmenters] [If applicable for this change] Tested for regression with Hadron.

@earlsprig earlsprig requested a review from a team as a code owner October 31, 2024 02:51
@seg-atlantis-prod
Copy link

Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors

@seg-atlantis-prod
Copy link 

Error parsing command: EOF found when expecting closing quote

@joe-ayoub-segment joe-ayoub-segment self-assigned this Oct 31, 2024
@joe-ayoub-segment
Copy link
Contributor

Thanks for the PR @earlsprig - I'll schedule for review.

joe-ayoub-segment
joe-ayoub-segment reviewed Nov 5, 2024
View reviewed changes
packages/destination-actions/src/destinations/sprig/identifyUser/index.ts
Object.prototype.hasOwnProperty.call(o.constructor.prototype, 'isPrototypeOf')
)

const flattenObj = (obj: { [k: string]: any }, keys = [] as string[]) : {[k: string]: any } => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The linter on my machine is complaining about the use of the any type. Could you use another type here please?

@joe-ayoub-segment
Copy link
Contributor

hi @earlsprig thanks for raising this PR.
I noticed that there's an existing Sprig Web Destination in our catalog, so I referenced it while reviewing this PR.

If possible should the field labels and descriptions the as similar as possible to those of the Web Integration?

I left some inline comments for you to respond to.

Best regards,
Joe

@joe-ayoub-segment
Copy link
Contributor

joe-ayoub-segment commented Nov 6, 2024 •
edited
Loading

hi @earlsprig - could you run yarn types and then commit the changes to your branch please?

Just the linting issue and we should be good to deploy.

@joe-ayoub-segment joe-ayoub-segment mentioned this pull request Nov 8, 2024
Merged
@joe-ayoub-segment
Copy link
Contributor

hi @earlsprig I've raised a separate PR with the changes I requested.
Please see #2574
Will deploy the above code next Tuesday.
Closing this PR.

@earlsprig
Copy link
Author

hi @earlsprig I've raised a separate PR with the changes I requested. Please see #2574 Will deploy the above code next Tuesday. Closing this PR.

Hey @joe-ayoub-segment - apologies for the delay here I was pulled on to some other projects this week. Appreciate the new PR. Let me know if there's any other action you need from my end?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joe-ayoub-segment joe-ayoub-segment joe-ayoub-segment left review comments

Assignees

@joe-ayoub-segment joe-ayoub-segment

Labels
deploy:push deploy:registration mode:cloud partner-eng-review (cloud) team:external
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@earlsprig @joe-ayoub-segment