🎉 New Source - Xero [python cdk] #18666
Conversation
|
|
github-actions
bot
added
area/connectors
There was a problem hiding this comment.
Hello @alkali, Marcos from Airbyte here 👋 . We received more than 25 new contributions along the weekend. One is yours 🎉 thank so much for! Our team is limited and maybe the review process can take longer than expected. As described in the Airbyte's Hacktoberfest your contribution was submitted before November 2nd and it is eligible to win the prize. The review process will validate other requirements. I ask to you patience until someone from the team review it.
Because I reviewed some contributions for Hacktoberfest so far I saw some common patterns you can check in advance:
- Make sure you have added connector documentation to
/docs/integrations/ - Remove the file
catalogfrom/integration_tests - Edit the
sample_config.jsoninside/integration_tests - For the
configured_catalogyou can use onlyjson_schema: {} - Add title to all properties in the
spec.yaml - Make sure the
documentationUrlin thespec.yamlredirect to Airbyte's future connector page, eg: connector Airtable thedocumentationUrl: https://docs.airbyte.com/integrations/sources/airtable - Review now new line at EOF (end-of-file) for all files.
If possible send to me a DM in Slack with the tests credentials, this process will make easier to us run integration tests and publish your connector. If you only has production keys, make sure to create a bootstrap.md explaining how to get the keys.
|
@alkali can you sign the CLA? |
yeah, I did it |
There was a problem hiding this comment.
A comment about custom function, @alkali do you have a test credential to share?
| def parse_date(value): | ||
| # Xero datetimes can be .NET JSON date strings which look like | ||
| # "/Date(1419937200000+0000)/" | ||
| # https://developer.xero.com/documentation/api/requests-and-responses | ||
| pattern = r"Date\((\-?\d+)([-+])?(\d+)?\)" | ||
| match = re.search(pattern, value) | ||
|
|
||
| iso8601pattern = r"((\d{4})-([0-2]\d)-0?([0-3]\d)T([0-5]\d):([0-5]\d):([0-6]\d))" | ||
|
|
||
| if not match: | ||
| iso8601match = re.search(iso8601pattern, value) | ||
| if iso8601match: | ||
| try: | ||
| return datetime.strptime(value) | ||
| except Exception: | ||
| return None | ||
| else: | ||
| return None | ||
|
|
||
| millis_timestamp, offset_sign, offset = match.groups() | ||
| if offset: | ||
| if offset_sign == "+": | ||
| offset_sign = 1 | ||
| else: | ||
| offset_sign = -1 | ||
| offset_hours = offset_sign * int(offset[:2]) | ||
| offset_minutes = offset_sign * int(offset[2:]) | ||
| else: | ||
| offset_hours = 0 | ||
| offset_minutes = 0 | ||
|
|
||
| return datetime.fromtimestamp((int(millis_timestamp) / 1000), tz=timezone.utc) + timedelta(hours=offset_hours, minutes=offset_minutes) | ||
|
|
There was a problem hiding this comment.
Can you add unit test for this function, also looks this function doesn't two different things, maybe you can break into two.
There was a problem hiding this comment.
IMO, it's not necessary to split these function in two as it's used for one certain thing: parse .NET JSON date format into datetime and these functions will never be used separately
I've added a unit test for this one, thanks for the catch
added them on |
| logger = logging.getLogger("airbyte") | ||
|
|
||
|
|
||
| class XeroCustomCuonnectionsOauth2Authenticator(Oauth2Authenticator): |
There was a problem hiding this comment.
You have a typo here - XeroCustomCuonnectionsOauth2Authenticator
There was a problem hiding this comment.
@alkali correct this and the connector is ready to go 🚀
| logger = logging.getLogger("airbyte") | ||
|
|
||
|
|
||
| class XeroCustomCuonnectionsOauth2Authenticator(Oauth2Authenticator): |
There was a problem hiding this comment.
@alkali correct this and the connector is ready to go 🚀
|
|
||
| ## Prerequisites | ||
|
|
||
| First of all you should create an application in [Xero development center](https://developer.xero.com/app/manage/). The only supported integration type is to use [Xero Custom Connections](https://developer.xero.com/documentation/guides/oauth2/custom-connections/developer) so you should choose it on creating your Xero App. |
There was a problem hiding this comment.
@alkali could you provide some context as to why this requires a Xero Custom Connection?
That's quite a limitation for some use cases for this connector, I would imagine.
Would someone be able to pick this up and adapt your code so that it could work with some extra short-lived token / refresh token logic?
There was a problem hiding this comment.
@stefl Xero access_token lives 30 minutes. Refresh_token lives 60 days, but if an application has used it for updating access_token, Xero will give you a new refresh_token and the old one will expire 30 minutes after usage. So it basically means that after refreshing access_token we should save a new refresh_token somewhere in the credentials (in the connector's config). This is impossible for now and I was waiting for #3990 to be resolved.
I have a workaround (I am actually using this Xero connector at my job for a while) - use an additional custom OAuth2Connector, which sends a callback with a new refresh_token to a particular URL, and some code at this particular URL updates the connector's config with new refresh_token. But I'm pretty sure this is not the way for a publicly released connector.
|
Hello! Your PR is approved but didn't have the time to publish and merge it this week. As you can check in Chris' comment all PRs submitted before 2-nov are eligible to win the prize. I'll be out of the office on Friday and return Monday to start publishing your contribution. Any question you can send a message in Have a good weekend and thank you for this amazing contribution for Hacktoberfest 🎉 |
|
/test connector=connectors/source-xero
Build FailedTest summary info: |
|
/test connector=connectors/source-xero
Build FailedTest summary info: |
|
/test connector=connectors/source-xero
Build FailedTest summary info: |
|
/test connector=connectors/source-xero
Build PassedTest summary info: |
|
/publish connector=connectors/source-xero
if you have connectors that successfully published but failed definition generation, follow step 4 here |
|
Hello @alkali can you please add your email to your github profile. This will allow us to contact you about your payout :). If you don't want to make your email public, please DM me on slack with the following:
Thanks! |
|
@RealChrisSean added an email to the profile, thx for noticing |
* xero connector 0.1.0 * docs and stuff * CR fixes: added sample_config.json and removed ununsed catalog.json * CR fixes: added unit test for parse_date * CR: typo fixed * add xero to source def * correct tests * update accpt test * correct abnormal state file * auto-bump connector version Co-authored-by: marcosmarxm <marcosmarxm@gmail.com> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com> Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com>
What
New source for Xero (as it's accounting software, it supports Accounting API only)
How
I've used Python CDK as there are some data transformations.
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret./gradlew :airbyte-integrations:connectors:<name>:integrationTest.README.mdbootstrap.md. See description and examplesdocs/integrations/<source or destination>/<name>.mdincluding changelog. See changelog exampledocs/integrations/README.mdairbyte-integrations/builds.mdAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>command is passing/publishcommand described hereTests
Unit
Acceptance