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.

Sign up for GitHub

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

Jump to bottom

🎉 Source Mixpanel low code migration #36724

Merged
merged 60 commits into from
May 8, 2024
Merged

🎉 Source Mixpanel low code migration #36724

merged 60 commits into from
May 8, 2024

Conversation

midavadim
Copy link
Contributor

@midavadim midavadim commented Apr 1, 2024 •
edited
Loading

@vercel Vercel
Copy link

vercel bot commented Apr 1, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview May 7, 2024 8:15pm

@octavia-squidington-iv octavia-squidington-iv requested review from a team April 1, 2024 17:18
@alafanechere alafanechere added the low-code-migration This connector has been migrated to the low-code CDK label Apr 3, 2024
@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Apr 12, 2024
@natikgadzhi
Copy link
Contributor

In #37833, someone wants to use composite PKs for Members stream, and that change, if it makes sense, would be breaking. @midavadim, should we roll it into this PR so we avoid two breaking changes on a connector in a row?

Will this PR be a breaking change on it's own, or a clean migration?

@RobinHerzog
Copy link

In #37833, someone wants to use composite PKs for Members stream, and that change, if it makes sense, would be breaking. @midavadim, should we roll it into this PR so we avoid two breaking changes on a connector in a row?

Will this PR be a breaking change on it's own, or a clean migration?

Does Airbyte has usage tracking over streams ? I think I saw segment in the logs

I understand it's a breaking change but it's weird no-one every reported it. Currently it's not possible to use this specific stream as it gives wrong answer.

@natikgadzhi
Copy link
Contributor

Usage tracking: yes, for Cloud, but I'm not entirely sure about per-stream data in OSS installations. We'll need to take a look.

You'd be surprised. ;)

girarda
girarda requested changes May 6, 2024
View reviewed changes
Copy link
Contributor

@girarda girarda left a comment

Choose a reason for hiding this comment

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

there are a few unanswered questions.

main concerns:

  • there's a lot of custom components. Are they all necessary?
  • There's a strange incremental_sync component. how is it expected to work?

airbyte-integrations/connectors/source-mixpanel/source_mixpanel/components.py Show resolved Hide resolved
airbyte-integrations/connectors/source-mixpanel/source_mixpanel/components.py Show resolved Hide resolved
airbyte-integrations/connectors/source-mixpanel/source_mixpanel/components.py Show resolved Hide resolved
airbyte-integrations/connectors/source-mixpanel/source_mixpanel/components.py Show resolved Hide resolved
airbyte-integrations/connectors/source-mixpanel/source_mixpanel/components.py Show resolved Hide resolved
airbyte-integrations/connectors/source-mixpanel/source_mixpanel/components.py Show resolved Hide resolved
airbyte-integrations/connectors/source-mixpanel/source_mixpanel/components.py
from .streams.engage import EngageSchema


class MixpanelHttpRequester(HttpRequester):
Copy link
Contributor

Choose a reason for hiding this comment

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

bump

airbyte-integrations/connectors/source-mixpanel/source_mixpanel/components.py Show resolved Hide resolved
airbyte-integrations/connectors/source-mixpanel/source_mixpanel/components.py Show resolved Hide resolved
airbyte-integrations/connectors/source-mixpanel/source_mixpanel/manifest.yaml Outdated Show resolved Hide resolved
@midavadim
Copy link
Contributor Author

there are a few unanswered questions.

main concerns:

  • there's a lot of custom components. Are they all necessary?
  • There's a strange incremental_sync component. how is it expected to work?
  1. I have re-checked that all components are needed. I created related issues to indicate problems with low code:
    https://github.com/airbytehq/airbyte-internal-issues/issues/7602
    https://github.com/airbytehq/airbyte-internal-issues/issues/7601
    https://github.com/airbytehq/airbyte-internal-issues/issues/7603
  2. I removed one odd incremental_sync component

@midavadim midavadim requested a review from girarda May 7, 2024 15:40
@midavadim
Copy link
Contributor Author

@natikgadzhi @RobinHerzog lets prohandle key for cohost members stream in separate PR

@midavadim
Merge branch 'master' into midavadim/36356-mixpanel-low-code-migration
e37672f 
# Conflicts:
#	airbyte-integrations/connectors/source-mixpanel/metadata.yaml
#	airbyte-integrations/connectors/source-mixpanel/poetry.lock
#	airbyte-integrations/connectors/source-mixpanel/pyproject.toml
#	airbyte-integrations/connectors/source-mixpanel/unit_tests/test_streams.py
#	docs/integrations/sources/mixpanel.md
girarda
girarda approved these changes May 7, 2024
View reviewed changes
Copy link
Contributor

@girarda girarda left a comment

Choose a reason for hiding this comment

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

approved pending the regression tests run passing https://github.com/airbytehq/airbyte/actions/runs/8993378634

@midavadim midavadim merged commit ef0ecc3 into master May 8, 2024
38 checks passed
@midavadim midavadim deleted the midavadim/36356-mixpanel-low-code-migration branch May 8, 2024 08:04
midavadim added a commit that referenced this pull request May 9, 2024
@midavadim
Revert "🎉 Source Mixpanel low code migration (#36724)"
a282057 
This reverts commit ef0ecc3.
@midavadim midavadim mentioned this pull request May 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@askarpets askarpets askarpets approved these changes

@RobinHerzog RobinHerzog RobinHerzog approved these changes

@girarda girarda girarda approved these changes

@natikgadzhi natikgadzhi Awaiting requested review from natikgadzhi

Assignees
No one assigned
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/mixpanel low-code-migration This connector has been migrated to the low-code CDK
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@midavadim @natikgadzhi @RobinHerzog @girarda @askarpets @alafanechere @octavia-squidington-iii