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

New branch for linkedin pages #1433

Merged
merged 12 commits into from
Apr 5, 2024
Merged

New branch for linkedin pages #1433

merged 12 commits into from
Apr 5, 2024

Conversation

jonwihl
Copy link
Contributor

@jonwihl jonwihl commented Mar 27, 2024

Description:

This connector uses the old python connector method and not the Estuary CDK. There were numerous issues with module imports and expected paths. For instance:

  • The schema folder had to exist at every level of the connector
  • The main module was imported as source_linkedin_pages.source_linkedin_pages in main.py

The branch only contains one commit because of a bad rebase I made on the previous branch. For more detailed changes you can review wihl/python-linkedin-pages. Its all kinda a mess, let me know if its even worth merging in this state.

Workflow steps:

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

Documentation links affected:

See estuary/flow#1433

Notes for reviewers:

(anything that might help someone review this PR)


This change is Reviewable

Z33DD added 4 commits March 28, 2024 15:50
…44dd243a809340d8c1c

Remote Repo URL: https://github.com/airbytehq/airbyte.git
Source name: c613853d10700c60d2edd44dd243a809340d8c1c
Source Commit ID: c613853d10700c60d2edd44dd243a809340d8c1c
Source Repo Prefix: airbyte-integrations/connectors/source-linkedin-pages/
Import Path: source-linkedin-pages/
License Type: MIT
License Path: ./airbyte-integrations/connectors/source-linkedin-pages/metadata.yaml

git-merge-subpath: c613853d10700c60d2edd44dd243a809340d8c1c airbyte-integrations/connectors/source-linkedin-pages source-linkedin-pages
source-linkedin-pages: Fix schema null types

source-linkedin-pages: Refactor LinkedIn API paths in source.py

source-linkedin-pages: Format code with ruff and isort
…ifying

tests: Add test capture files and credentials
@Z33DD Z33DD force-pushed the source-linkedin-pages branch from d29aa04 to 0284732 Compare March 28, 2024 18:53
@Z33DD Z33DD self-assigned this Mar 28, 2024
Copy link
Member

@williamhbaker williamhbaker left a comment

Choose a reason for hiding this comment

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

The tests are broken - I think you need to update the snapshot tests for the added OAuth2 support. Also made some inline comments about minor URL related changes we need.

Other than that, this looks great, and I think is just about ready to merge. Before we merge it, please do a QA test along these lines:

  1. Work with @jonwihl to add the connector to our production supabase tables, using the image tag built from this pull request (which will be available when you fix the tests)
  2. Set up a capture using the connector through the UI on dashboard.estuary.dev. Make sure that data is captured as expected, and checkpoint resumption works as expected when you restart the task by disabling it and then re-enabling it through the UI. Make sure it works with all possible authentication methods.
  3. Materialize the collections captured by the connector and make sure everything works as expected. Use one of the SQL-based materializations. I'd suggest running a local postgres or mysql instance via docker and getting a public address using ngrok.
  4. After merging and the v1 image is pushed from the main branch CI process, let @jonwihl know so he can update the version tag in supabase.

Also we need docs written for this. It is the developer's responsibility to put together docs for new connectors, or update docs based on changes. Please get a docs PR started against the estuary/flow repo and provide a link to that in this pull request. For an example of a docs pull request, see estuary/flow#1402

source-linkedin-pages/tests/snapshots/spec.stdout.json Outdated Show resolved Hide resolved
source-linkedin-pages/tests/snapshots/spec.stdout.json Outdated Show resolved Hide resolved
source-linkedin-pages/tests/snapshots/spec.stdout.json Outdated Show resolved Hide resolved
Copy link
Member

@williamhbaker williamhbaker left a comment

Choose a reason for hiding this comment

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

Made a couple more comments

source-linkedin-pages/source_linkedin_pages/spec.json Outdated Show resolved Hide resolved
source-linkedin-pages/tests/snapshots/spec.stdout.json Outdated Show resolved Hide resolved
@williamhbaker
Copy link
Member

Also, please link the docs PR in this PR description under the "Documentation links affected:" header

@Z33DD Z33DD requested a review from williamhbaker April 3, 2024 18:18
Copy link
Member

@williamhbaker williamhbaker left a comment

Choose a reason for hiding this comment

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

LGTM

@Z33DD Z33DD force-pushed the source-linkedin-pages branch from 0940c75 to 2abb9fc Compare April 4, 2024 21:40
@jonwihl jonwihl merged commit 6567726 into main Apr 5, 2024
49 of 55 checks passed
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.

3 participants