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

🎉 New source: Coda [python cdk] #18675

Merged
merged 25 commits into from
Nov 17, 2022

Conversation

himanshuc3
Copy link
Contributor

@himanshuc3 himanshuc3 commented Oct 30, 2022

What

The following PR adds Python-CDK based Coda Connector. For more context, please refer to airbytehq/connector-contest#210.

How

Using Python-CDK based connector for Coda streams, following streams have been baked in:

  • categories
  • docs
  • formulas
  • pages
  • permissions
  • tables

Recommended reading order

  1. spec.yaml
  2. schemas/ folder

🚨 User Impact 🚨

Not applicable

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

  • Community member? Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
    • docs/integrations/README.md
    • airbyte-integrations/builds.md
  • PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • If new credentials are required for use in CI, add them to GSM. Instructions.
  • /test connector=connectors/<name> command is passing
  • New Connector version released on Dockerhub by running the /publish command described here
  • After the connector is published, connector added to connector index as described here
  • Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

@CLAassistant
Copy link

CLAassistant commented Oct 30, 2022

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the area/connectors Connector related issues label Oct 30, 2022
@marcosmarxm marcosmarxm changed the title New source: coda 🎉 New source: Coda Oct 31, 2022
Copy link
Member

@marcosmarxm marcosmarxm left a comment

Choose a reason for hiding this comment

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

Hello @himanshuc3, 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 catalog from /integration_tests
  • Edit the sample_config.json inside /integration_tests
  • For the configured_catalog you can use only json_schema: {}
  • Add title to all properties in the spec.yaml
  • Make sure the documentationUrl in the spec.yaml redirect to Airbyte's future connector page, eg: connector Airtable the documentationUrl: 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.

@marcosmarxm marcosmarxm changed the title 🎉 New source: Coda 🎉 New source: Coda [python cdk] Oct 31, 2022
@marcosmarxm
Copy link
Member

Hello! I'm going to be out of the office this Friday and won't be able to review your contribution again today, I return next Monday. So far, most contributions look solid and are almost done to be approved. As said in Chris' comment all contributions made before 2-November are eligible to receive the prize and have 2 weeks to merge the contributions. But I ensure next week we're going to have your contribution merged. If you have questions about the implementation you can send them in #hacktoberfest-2022 in Airbyte's Slack.

Sorry the inconvenience and see you again next week, thank you so much for your contribution!

@himanshuc3
Copy link
Contributor Author

@marcosmarxm Thanks for the update. I would ensure most of the items are checked off by then since I needed to review the list of items mentioned by you.

@marcosmarxm
Copy link
Member

@himanshuc3 did you have time to check the list?

@himanshuc3
Copy link
Contributor Author

apologies, didn't get bandwith to work on this due to university curriculum.
Will try to wind up the list and refectoring today, so expect it to be ready to review by tomorrow.

@himanshuc3 himanshuc3 force-pushed the himanshu/code-source branch from 4062f75 to 797c495 Compare November 8, 2022 06:47
@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Nov 8, 2022
@himanshuc3
Copy link
Contributor Author

@marcosmarxm Verified and pushed changes mentioned in the checklist.
Having problems building and testing in UI because of java compilation error with gradle. Debugging that at the moment.

@marcosmarxm
Copy link
Member

I'll take a look after today @himanshuc3

Copy link
Member

@marcosmarxm marcosmarxm left a comment

Choose a reason for hiding this comment

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

Some comments, @himanshuc3 there are some missing code to finish the connector. Let me know if you need assistance with it.

Comment on lines 1 to 7
# TODO: Define your stream schemas
Your connector must describe the schema of each stream it can output using [JSONSchema](https://json-schema.org).

The simplest way to do this is to describe the schema of your streams using one `.json` file per stream. You can also dynamically generate the schema of your stream in code, or you can combine both approaches: start with a `.json` file and dynamically add properties to it.

The schema of a stream is the return value of `Stream.get_json_schema`.

Copy link
Member

Choose a reason for hiding this comment

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

Remove this file.

Comment on lines 2 to 11
"streams": [
{
"stream": {
"name": "docs",
"json_schema": {
"$schema": "http://json-schema.org/draft-07/schema#",
"type": "object",
"properties": {
"access_key": {
"type": "string"
Copy link
Member

Choose a reason for hiding this comment

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

Remove this file.

Comment on lines 181 to 197
@property
def cursor_field(self) -> str:
"""
TODO
Override to return the cursor field used by this stream e.g: an API entity might always use created_at as the cursor field. This is
usually id or date based. This field's presence tells the framework this in an incremental stream. Required for incremental.

:return str: The name of the cursor field.
"""
return []

def get_updated_state(self, current_stream_state: MutableMapping[str, Any], latest_record: Mapping[str, Any]) -> Mapping[str, Any]:
"""
Override to determine the latest state after reading the latest record. This typically compared the cursor_field from the latest record and
the current state and picks the 'most' recent cursor. This is how a stream's state is determined. Required for incremental.
"""
return {}
Copy link
Member

Choose a reason for hiding this comment

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

This probably won't work to getting an incremental sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haven't yet implemented it, is it required for the creation of a data source. It could probably better if it's done in another pr, incremental feature addition ;)

Copy link
Member

Choose a reason for hiding this comment

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

Remove it if isn't used.

Copy link
Member

@marcosmarxm marcosmarxm left a comment

Choose a reason for hiding this comment

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

@himanshuc3 please implement the pagination for streams

Comment on lines 46 to 53
def request_params(
self, stream_state: Mapping[str, Any], stream_slice: Mapping[str, any] = None, next_page_token: Mapping[str, Any] = None
) -> MutableMapping[str, Any]:
"""
TODO: Override this method to define any query parameters to be set. Remove this method if you don't need to define request params.
Usually contains common params e.g. pagination size etc.
"""
return {}
Copy link
Member

Choose a reason for hiding this comment

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

The source is lacking of pagination which is required even for full refresh sync mode. Please check the API documentation and implement it.

@marcosmarxm
Copy link
Member

@himanshuc3 please also share the output of integration tests.

@marcosmarxm
Copy link
Member

marcosmarxm commented Nov 16, 2022

/publish connector=connectors/source-coda

🕑 Publishing the following connectors:
connectors/source-coda
https://github.com/airbytehq/airbyte/actions/runs/3483957354


Connector Did it publish? Were definitions generated?
connectors/source-coda

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

Copy link
Member

@marcosmarxm marcosmarxm left a comment

Choose a reason for hiding this comment

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

Thanks @himanshuc3

@himanshuc3
Copy link
Contributor Author

thanks @marcosmarxm for helping me with any doubts. Hope to return and contribute further whenever bandwidth is available.

@himanshuc3
Copy link
Contributor Author

not sure why the Add labels github action is failing, It throws the following error:

GraphqlResponseError: Request failed due to following response errors:
 - The `ProjectNext` API is deprecated in favour of the more capable `ProjectV2` API. Follow the ProjectV2 guide at https://github.blog/changelog/2022-06-23-the-new-github-issues-june-23rd-update/, to find a suitable replacement.
    at /home/runner/work/airbyte/airbyte/.private-action/webpack:/workflow-actions/node_modules/@octokit/graphql/dist-node/index.js:81:1
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
Error: Error: The process '/usr/local/bin/node' failed with exit code 1

@marcosmarxm

@marcosmarxm marcosmarxm merged commit acea2a5 into airbytehq:master Nov 17, 2022
@marcosmarxm
Copy link
Member

Thanks @himanshuc3 for the contribution

@apolo-damasco
Copy link

Hi all, thank you for this integration!!

When will we see it in Airbyte Cloud?

@marcosmarxm
Copy link
Member

@YowanR can you check this request?

@YowanR
Copy link
Contributor

YowanR commented Nov 22, 2022

Thanks @marcosmarxm!

@apolo-damasco We are working through our backlog to get connectors onto Airbyte Cloud. If you want to see this connector on Cloud sooner, it would help if you could give this comment a 👍. Thanks!

@apolo-damasco
Copy link

So good news, the connection is already showing up and working!
However, is lacking the ability to sync the rows of tables which is the main use case for this integration :(
At the moment is only possible to sync metadata of your docs...
@himanshuc3 any chance you could implement it?

@himanshuc3
Copy link
Contributor Author

himanshuc3 commented Dec 2, 2022 via email

akashkulk pushed a commit that referenced this pull request Dec 2, 2022
* init coda

* fix: Streams added

* fix: Common review comments

* fix: definition

* fix: Remove sample files

* fix: pagination

* integration test configs

* fix: remove validations

* fix: unit test

* fix: unit test removed

* fix: unit tests added

* linting

* fix: pytest to 6.2.5

* remove columns

* update connector

* add coda to source def

* remove coda from source def

* readd missing file

* rollback change to index.html

* correct some files

* rollback change in cart.com file

* fix sample files

* run format

* auto-bump connector version

Co-authored-by: marcosmarxm <marcosmarxm@gmail.com>
Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
@apolo-damasco
Copy link

@himanshuc3 any news on this?
@YowanR is there any chance the team could work on this? This is so close to being functional... it would be a waste to leave it like this.

@YowanR
Copy link
Contributor

YowanR commented Mar 17, 2023

@apolo-damasco I think we should create a new issue to track what you are asking above. Can you do this and tag me? I'll work on getting prioritized

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

8 participants