-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Source Close.com: migration to low-code #18968
Source Close.com: migration to low-code #18968
Conversation
# Declarative Source | ||
class SourceCloseCom(YamlDeclarativeSource): | ||
def __init__(self): | ||
super().__init__(**{"path_to_yaml": "close_com.yaml"}) |
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.
super().__init__(**{"path_to_yaml": "close_com.yaml"}) | |
super().__init__(path_to_yaml="close_com.yaml") |
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 is an auto-generated code that no need to touch. This is how all the Source
classes should look like
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.
🤷 okay
@@ -12,5 +12,5 @@ RUN pip install . | |||
ENV AIRBYTE_ENTRYPOINT "python /airbyte/integration_code/main.py" | |||
ENTRYPOINT ["python", "/airbyte/integration_code/main.py"] | |||
|
|||
LABEL io.airbyte.version=0.1.0 | |||
LABEL io.airbyte.version=0.1.1 |
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.
I think a minor version should be bumped, not patch
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.
Nothing changed at all from functional perspective. Moreover we are using the same code as we used before, but just run it in another way. Should we really bump it?
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.
but it does not look like a bugfix as well :)
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.
Agreed
"incoming_sms_tasks", "zoom_connected_accounts", "custom_email_connected_accounts", | ||
"google_connected_accounts", "send_as", "email_sequences", "dialer", "email_bulk_actions", | ||
"sequence_subscription_bulk_actions", "delete_bulk_actions"] | ||
empty_streams: |
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.
there should be no empty streams if we're going to certify this connector to beta
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.
I was trying to avoid empty streams but didn't succeed. We cannot to populate some tasks from our side and send_as
requires payment. It also makes impossible for now to migrate SAT config to new version
@@ -14,10 +14,24 @@ tests: | |||
basic_read: |
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.
could you please migrate SAT config to the new version?
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.
New version does not support empty_streams
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.
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.
Done
airbyte-integrations/connectors/source-close-com/source_close_com/single_stream_slicer.py
Outdated
Show resolved
Hide resolved
|
||
|
||
@dataclass | ||
class OffsetIncrementWorkaround(PaginationStrategy, JsonSchemaMixin): |
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.
could you please add a docstring explaining the difference between this class and other paginating strategies?
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.
Done
$ref: "*ref(definitions.incremental_stream)" | ||
stream_cursor_field: "date_created" | ||
$options: | ||
primary_key: "id" |
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 primary key should be defined outside the options
parameter
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.
I can put it here as well, if I need to specify it by primary_key
per stream. But you are right, this key isn't used and was finally defined globally in base stream. It should be removed from here
airbyte-integrations/connectors/source-close-com/source_close_com/close_com.yaml
Outdated
Show resolved
Hide resolved
what's the purpose of not removing the python code? |
It is just an agreement |
d6957c9
to
07f3b3f
Compare
/test connector=connectors/source-close-com
Build PassedTest summary info:
|
|
||
def __post_init__(self, options: Mapping[str, Any]): | ||
self._offset = 0 | ||
self.page_size = InterpolatedString.create(self.page_size, options=options) |
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.
@roman-yermilov-gl how about updating the paginator's field to be an interpolated string instead of duplicating the class for a one-line change?
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.
It may affect existing connectors that using page_size as integer?
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.
page_size should always return an int, but the underlying field could be Union[InterpolatedString, str, int]
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.
Removed custom component, but need the following PR to be accepted:
#19646
|
||
|
||
@dataclass | ||
class SingleStreamSlicer(SingleSlice): |
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.
is there a benefit to using a single slice instead of chunking the time range like DatetimeStreamSlicer does? Using multiple slices is nice because it gives us checkpointing for free
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.
There are streams that require a very small step, a millisecond or less. It looks like I cannot avoid customization
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.
Fine with me if it has to be custom, but I'd like to better understand the limitation of the implementation in the CDK.
The DatetimeStreamSlicer's update_cursor
method also updates the cursor based on the last record read, so I would expect it to fetch the same records even if they are in different slices
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.
Figured out how to avoid custom Slicer, so custom component is removed
94c3ad6
to
e9e37a7
Compare
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.
love it! Approved pending #19646
field_name: "_skip" | ||
pagination_strategy: | ||
type: OffsetIncrement | ||
page_size: "{{ options['items_per_page'] }}" |
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.
nice
/test connector=connectors/source-close-com
Build FailedTest summary info:
|
9d79545
to
d373d86
Compare
/test connector=connectors/source-close-com
Build PassedTest summary info:
|
/publish connector=connectors/source-close-com
if you have connectors that successfully published but failed definition generation, follow step 4 here |
What
This is a migration to Low-code
How
Old code is still present but replaced with new Source Declarative class in
__init__.py
. Made custom Slicer and Pagination to support custom needs of the connector