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

feature(partner-sdk): added python destination example #30

Merged
merged 8 commits into from
Apr 16, 2024

Conversation

fivetran-abdulsalam
Copy link
Contributor

@fivetran-abdulsalam fivetran-abdulsalam commented Apr 1, 2024

Links: https://fivetran.height.app/T-654507

Description

Added new python destination example.
Below is the console output:
Screenshot 2024-04-01 at 4 22 18 PM

Copy link
Collaborator

@manjutapali manjutapali left a comment

Choose a reason for hiding this comment

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

Looks good

examples/destination/python/destination/main.py Outdated Show resolved Hide resolved
examples/destination/python/destination/build.sh Outdated Show resolved Hide resolved
Copy link
Collaborator

@ediril ediril left a comment

Choose a reason for hiding this comment

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

Let get rid of the inner destination folder. All the files should go into examples/destination/python

examples/destination/python/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@ediril ediril left a comment

Choose a reason for hiding this comment

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

LGTM, except for the minor comments

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file should have executable flag enabled:

chmod +x run.sh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

enabled executable flag

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file should have executable flag enabled:

chmod +x build.sh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

enabled executable flag

@@ -0,0 +1,13 @@
#!/bin/bash
python3 -m venv destination_run
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should display some messages for the steps, otherwise it just sits there, doing a bunch of stuff, but I have no idea what's happening..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment for each step

@@ -0,0 +1,8 @@
grpcio==1.62.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should match the versions we are showing in the development guide. Let's use 1.61.1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems version 1.61.1 doesn't exists:
Screenshot 2024-04-15 at 5 15 13 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm interesting, I guess java versions don't necessarily match python versions. Let's go with 1.60.1 then.

@fivetran-abdulsalam fivetran-abdulsalam merged commit 44f8fb2 into main Apr 16, 2024
1 check passed
@fivetran-abdulsalam fivetran-abdulsalam deleted the test-destiontion branch April 16, 2024 05:11
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