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

Low-Code CDK: make DatetimeStreamSlicer.step InterpolatedString #21930

Merged
merged 5 commits into from
Jan 31, 2023

Conversation

grubberr
Copy link
Contributor

@grubberr grubberr commented Jan 26, 2023

Signed-off-by: Sergey Chvalyuk grubberr@gmail.com

What

  • make DatetimeStreamSlicer.step as InterpolatedString
  • we interpolate early on __post__init, it allows to interpolate only {{ config }}, but one the other hand we fail fast if provided value is not correct

Signed-off-by: Sergey Chvalyuk <grubberr@gmail.com>
@grubberr grubberr requested a review from a team as a code owner January 26, 2023 18:20
@octavia-squidington-iv octavia-squidington-iv added the CDK Connector Development Kit label Jan 26, 2023
@grubberr grubberr self-assigned this Jan 26, 2023
Signed-off-by: Sergey Chvalyuk <grubberr@gmail.com>
@@ -1,5 +1,8 @@
# Changelog

## 0.25.1
Low-Code CDK: make DatetimeStreamSlicer.step as InterpolatedString
Copy link
Contributor

Choose a reason for hiding this comment

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

As part of the new release process, the version is updated in the new GitHub workflow. You can check https://airbytehq-team.slack.com/archives/C02UF50V9HA/p1674655636577679?thread_ts=1674654710.391809&cid=C02UF50V9HA for more information.

An example of the issue is this: you have another PR at the same time that has the same release version. This will clash i.e. one of the release will fail. To avoid this, we should always release on master. This allows to have one source of truth for the version and not multiple (one per branch)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@grubberr grubberr requested a review from maxi297 January 30, 2023 09:15
@sherifnada sherifnada removed the request for review from alafanechere January 31, 2023 06:43
@lazebnyi lazebnyi requested a review from girarda January 31, 2023 10:14
Copy link
Contributor

@maxi297 maxi297 left a comment

Choose a reason for hiding this comment

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

I don't see any issue with this MR. However, I'm curious in what cases this could be useful. Can you provide an example?

@grubberr
Copy link
Contributor Author

grubberr commented Jan 31, 2023

Hello,
I am working on rewriting source-Twilio from old to low-code CDK
Here is an example:
https://github.com/airbytehq/airbyte/pull/21446/files#diff-5b7ed3cb893f1b04c3c4ce89ac5b7abb7de4e9d12d9663a81f612c3f75bfdebdR49

tag: @maxi297

@maxi297
Copy link
Contributor

maxi297 commented Jan 31, 2023

Thanks a lot for the example @grubberr ! The more I know, the better I feel haha

Copy link
Contributor

@maxi297 maxi297 left a comment

Choose a reason for hiding this comment

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

Just adding a comment to approve. Good job!

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

Successfully merging this pull request may close these issues.

None yet

3 participants