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.

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Luishfs
Copy link
Collaborator

@Luishfs Luishfs commented Mar 13, 2024

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 Reviewable

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"
@Luishfs Luishfs self-assigned this Mar 13, 2024
Copy link
Member

@jgraettinger jgraettinger left a 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

connector:
image: "ghcr.io/estuary/source-salesforce:v1"
config: config.yaml
bindings:
Copy link
Member

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"
Copy link
Member

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.

Copy link
Member

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)?

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

Successfully merging this pull request may close these issues.

2 participants