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.
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
🎉 source Salesloft: added new emails incremental stream #11239
🎉 source Salesloft: added new emails incremental stream #11239
Conversation
Thank you @yannibenoit for the contribution. I'll run the acceptance tests and go for a first review asap. |
/test connector=connectors/source-salesloft
|
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.
Thank you @yannibenoit for this contribution! I made a small suggestion about nullable fields.
"type": "object", | ||
"properties": { | ||
"id": { | ||
"type": ["null", "integer"] |
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.
"type": ["null", "integer"] | |
"type": "integer" |
Please double-check which fields are nullable, here for example id
is probably not nullable. The updated_at
field should also not be nullable if you want to use it as the cursor field. If Salesloft does not provide this information please at lease set the following field as not nullable:
id
, created_at
, updated_at
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.
@alafanechere sorry, bad copy paste ....
do you want me to apply the changes ? 👍
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.
Yes please @yannibenoit
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.
but @alafanechere we have a similar problem on other schemas 🤔
id, created_at, updated_at are nullable on the following schemas:
- cadences.json
- people.json
- cadence_memberships.json
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.
Made the changes for all schema , let's try out
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.
Could you please only change your stream's schema to make this PR atomic, and maybe open a second one for other streams?
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.
yep done ✅
i have to make another pull requests for new endpoints/streams anyway so best practice is to make one PR per endpoint + 1 PR to adjust the schema of existing streams
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.
As this PR is aimed at adding a new stream I find it better to not refactor the other streams in the same PR.
If you're working on PR to change the existing stream feel free to change their schema in this new PR too.
Or if you prefer to align all the schemas in a single PR feel free to do it too.
/publish connector=connectors/source-salesloft
|
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.
Thank you for the changes, publishing now 🎉
What
New source added for Salesloft integration -> activities/emails.json
How
emails.json
IncrementalSalesloftStream
class ->Emails
streams
method inSourceSalesloft
class to add new class in returntest_source.py
to allow one more stream when unit testingRecommended reading order
emails.json
source.py
test_source.py
🚨 User Impact 🚨
New stream will be available in connector settings
New tables will be available in destination
**
**
Pre-merge Checklist
Expand the relevant checklist and delete the others.
Updating a connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereTests
Unit
Put your unit tests output here.
Integration
Put your integration tests output here.
Acceptance
Put your acceptance tests output here.