-
Notifications
You must be signed in to change notification settings - Fork 17
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
Added new connector: Source-Salesforce #1367
base: main
Are you sure you want to change the base?
Conversation
Fixed flow.yaml file configs
Remote Repo URL: [email protected]:airbytehq/airbyte.git Source name: 0f887bc Source Commit ID: 0f887bc143ee14726983816c9972b9398e500bc0 Source Repo Prefix: airbyte-integrations/connectors/source-salesforce/ Import Path: source-salesforce/ License Type: MIT License Path: airbyte-integrations/connectors/source-salesforce/metadata.yaml git-merge-subpath: 0f887bc143ee14726983816c9972b9398e500bc0 airbyte-integrations/connectors/source-salesforce source-salesforce
Removed integration tests Due to the source-salesforce connector having a big number of schema files AND the connector not running capture tests, the necessity of schema files is undermined. Since that, this commit rebases into removing those
At this point in time, all unit_tests shall pass, only snapshot ones "fail"
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.
Looking close. My remaining concerns are around the differences introduced in the discovery and spec snapshots. See comments below
source-salesforce/test.flow.yaml
Outdated
connector: | ||
image: "ghcr.io/estuary/source-salesforce:v1" | ||
config: config.yaml | ||
bindings: |
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.
nit: can leave this empty, as bindings: []
.
Rationale: when editing / testing locally it's trivial to restore these with flowctl raw discover
. Since we don't have a snapshot test that verifies captured documents, these aren't necessary so probably better not to have them.
"refresh_token" | ||
], | ||
"type": "object", | ||
"x-oauth2-provider": "salesforce" |
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.
This x-oauth2-provider bit is important and needs to be preserved.
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.
The amount of change in this file is concerning -- it's too large to even review in GitHub. Can you discuss the nature of the changes here? If you re-create this snapshot, after having switched to the estuary-cdk, are there fewer changes (because it was re-orderings introduced by the older flow-sdk
that have since reverted to their original snapshot order)?
Description:
(Describe the high level scope of new or changed features)
Notes for reviewers:
(anything that might help someone review this PR)
This change is