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 Salesforce source #1355

Closed
wants to merge 11 commits into from
Closed

Conversation

Luishfs
Copy link
Collaborator

@Luishfs Luishfs commented Mar 11, 2024

Description:

(Describe the high level scope of new or changed features)

Workflow steps:

(How does one use this feature, and how has it changed)

Documentation links affected:

(list any documentation links that you created, or existing ones that you've identified as needing updates, along with a brief description)

Notes for reviewers:

(anything that might help someone review this PR)
@jgraettinger


This change is Reviewable

Luishfs added 8 commits March 8, 2024 14:50
Fixed flow.yaml file configs
Remote Repo URL: git@github.com: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 11, 2024
@@ -48,6 +50,9 @@ jobs:
type: capture
version: v1
usage_rate: "0.0"
- name: source-salesforce
type: capture
version: v2
Copy link
Member

Choose a reason for hiding this comment

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

this will need usage_rate: "1.0"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in a1c34e9

@@ -19,14 +19,14 @@ def time_sleep_mock(mocker):

@pytest.fixture(scope="module")
def bulk_catalog():
with open("unit_tests/bulk_catalog.json") as f:
with open("source-salesforce/tests/bulk_catalog.json") as f:
Copy link
Member

Choose a reason for hiding this comment

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

does this scoping change if you add an empty __init__.py to this tests/ folder ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The __init__.py is already at the folder =P

@@ -0,0 +1,132 @@
# Salesforce Source
Copy link
Member

Choose a reason for hiding this comment

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

This readme can be removed, as it's relevant only for Airbyte.

Better to not have it, then to have one that's wrong.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed in a1c34e9

Copy link
Member

Choose a reason for hiding this comment

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

This can be removed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed in a1c34e9

@@ -0,0 +1,24 @@
data:
Copy link
Member

Choose a reason for hiding this comment

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

this can be removed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed in a1c34e9


[tool.poetry.dependencies]
python = ">=3.11,<3.12"
flow-sdk = {path="../python", develop=true}
Copy link
Member

Choose a reason for hiding this comment

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

This is using the older flow-sdk, which we've now replaced with the estuary-cdk.

Switching everything over has been a work in progress that's happened concurrently with your efforts here (see this just-landed PR). Would you please update accordingly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Modified in a1c34e9

- "-m"
- source-salesforce
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.

Please remove these, leaving just bindings: [].

They're already covered by the discovery snapshot.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated in a1c34e9

],
"title": "Salesforce Source Spec",
"type": "object"
},
"documentationUrl": "https://go.estuary.dev/AyWXIh",
"documentationUrl": "https://docs.airbyte.com/integrations/sources/salesforce",
Copy link
Member

Choose a reason for hiding this comment

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

We should restore this. It's threaded through in __main__.py as I recall.

Copy link
Collaborator Author

@Luishfs Luishfs Mar 11, 2024

Choose a reason for hiding this comment

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

restored in e039e79

],
"type": "object",
"x-oauth2-provider": "salesforce"
"auth_type": {
Copy link
Member

Choose a reason for hiding this comment

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

This schema change is ... wrong. I'm not sure what's happened here. Note that auth_type is top-level, but should be nested under credentials -- compare to spec.yaml.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it doesn't make sense, probably lost it in the rebase, will replace it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

restored in e039e79

Copy link
Member

Choose a reason for hiding this comment

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

Ouch, there's an awful lot of churn in here in switching over the connector -- so much that Github won't even load it.

What's the nature of the changes that are happening?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, the salesforce connector generates its own schema when running, whats going on is that the discovery is generating all 600 schemas ( 128.793 lines ) and displaying them

Copy link
Member

Choose a reason for hiding this comment

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

Understood, but I was talking about the changes in this snapshot between the baseline (from snapshots of the docker image) and this commit which switches over to the imported connector. I would not expect nearly as much churn here.

It's possible it may just be re-orderings and the diff will get much smaller after switching to the estuary-cdk, which preserves the sorted order used by the original Airbyte connector (while the flow-sdk randomizes it).

Luishfs added 2 commits March 11, 2024 16:29
Correct version was probably lost during rebase, this represents the spec state after
correcting spec.yaml file
@Luishfs Luishfs force-pushed the salesforce-source branch from 13ae043 to e039e79 Compare March 11, 2024 19:38
@Luishfs
Copy link
Collaborator Author

Luishfs commented Mar 13, 2024

Closing due to review, this PR needs rebasing.

@Luishfs Luishfs closed this Mar 13, 2024
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