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

update CDK airbyte protocol models to fix master build #12829

Merged
merged 1 commit into from
May 13, 2022
Merged

update CDK airbyte protocol models to fix master build #12829

merged 1 commit into from
May 13, 2022

Conversation

pedroslopez
Copy link
Contributor

@pedroslopez pedroslopez commented May 12, 2022 •
edited
Loading

What

The airbyte protocol was updated in #12586, which is now causing the Connectors Base build to fail due to some generated files within the CDK.

How

Re-generate the airbyte_protocol.py file.

Since we're not using this new state structure in the CDK yet a release shouldn't be required.

@github-actions github-actions bot added the CDK Connector Development Kit label May 12, 2022
@pedroslopez pedroslopez marked this pull request as ready for review May 12, 2022 22:37
benmoriceau
benmoriceau reviewed May 12, 2022
View reviewed changes
Copy link
Contributor

@benmoriceau benmoriceau left a comment

Choose a reason for hiding this comment

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

LGTM, but it will be nice to have someone more familliar with this file to do a review

evantahler
evantahler reviewed May 12, 2022
View reviewed changes
Copy link
Contributor

@evantahler evantahler left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I guess since this is auto-generated from the Java code... this seems reasonable.

Can you test a few of the GA connectors against this update to confirm that the change really isn't breaking (use the / commands on this PR)? AirbyteStateMessage did gain some properties that it didn't have before.

marcosmarxm
marcosmarxm reviewed May 12, 2022
View reviewed changes
airbyte-cdk/python/airbyte_cdk/models/airbyte_protocol.py

state_type: Optional[AirbyteStateType] = None
data: Optional[Dict[str, Any]] = Field(None, description="(Deprecated) the state data")
global_: Optional[AirbyteStateBlob] = Field(None, alias="global")
Copy link
Member

Choose a reason for hiding this comment

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

@pedroslopez the global_ is correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! Yeah... global is a reserved word in python, so the generator added the underscore, making it global_.

@pedroslopez
Copy link
Contributor Author

@evantahler I was not able to run the /test commands here because the connectors would just pull the latest version of the CDK from pypi and not consider these changes (similar issue here #12799). I was able to run salesforce and google sheets locally, and CDK unit tests are passing. All fields that were added are optional, so shouldn't be an issue.

@pedroslopez pedroslopez merged commit b8703ea into master May 13, 2022
@pedroslopez pedroslopez deleted the pedroslopez/fix-generated-cdk-protocol branch May 13, 2022 02:38
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request May 13, 2022
Merged
@pedroslopez pedroslopez mentioned this pull request May 17, 2022
Merged
suhomud pushed a commit that referenced this pull request May 23, 2022
@pedroslopez @suhomud
update CDK airbyte protocol models to fix master build (#12829)
215b8d6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcosmarxm marcosmarxm marcosmarxm left review comments

@benmoriceau benmoriceau benmoriceau left review comments

@evantahler evantahler evantahler approved these changes

@Phlair Phlair Awaiting requested review from Phlair

Assignees
No one assigned
Labels
CDK Connector Development Kit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pedroslopez @evantahler @marcosmarxm @benmoriceau