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

Source Trello: enable specifying board ids in configuration #8183

Merged
merged 6 commits into from
Nov 29, 2021
Merged

Source Trello: enable specifying board ids in configuration #8183

merged 6 commits into from
Nov 29, 2021

Conversation

erdenezul
Copy link

@erdenezul erdenezul commented Nov 22, 2021 •
edited
Loading

What

  • Added configuration for allowing the user to specify board ids in configuration

How

If the user has specified board ids in UI, the source connector will not fetch board data again and again for every source.

Recommended reading order

  1. x.java
  2. y.python

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/SUMMARY.md
    • 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
  • Credentials added to Github CI. 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

Updating a connector

Community member or Airbyter

  • 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
    • Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
  • 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
  • Credentials added to Github CI. Instructions.
  • /test connector=connectors/<name> command is passing.
  • New Connector version released on Dockerhub by running the /publish command described here
  • After the new connector version is published, connector version bumped in the seed directory 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

Connector Generator

  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • If adding a new generator, add it to the list of scaffold modules being tested
  • The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
  • Documentation which references the generator is updated as needed.

@CLAassistant
Copy link

CLAassistant commented Nov 22, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the area/connectors Connector related issues label Nov 22, 2021
@github-actions github-actions bot added area/api Related to the api area/documentation Improvements or additions to documentation area/frontend area/platform issues related to the platform area/protocol area/scheduler area/server area/worker Related to worker CDK Connector Development Kit kubernetes normalization labels Nov 25, 2021
@github-actions github-actions bot removed area/api Related to the api area/frontend area/scheduler area/platform issues related to the platform area/server area/worker Related to worker normalization area/documentation Improvements or additions to documentation kubernetes CDK Connector Development Kit area/protocol labels Nov 25, 2021
@grubberr grubberr self-requested a review November 25, 2021 09:00
@sherifnada sherifnada mentioned this pull request Nov 29, 2021
Closed
40 tasks
@sherifnada sherifnada linked an issue Nov 29, 2021 that may be closed by this pull request
Source Trello: allow a list of board_id in config #7898
Closed
@grubberr
Copy link
Contributor

Airbyte bump version PR #8259

@antixar antixar removed the request for review from sherifnada November 29, 2021 17:02
@grubberr grubberr merged commit 72b2f8b into airbytehq:master Nov 29, 2021
@grubberr grubberr mentioned this pull request Nov 29, 2021
Merged
16 tasks
@jrhizor jrhizor mentioned this pull request Nov 29, 2021
Merged
@danieldiamond
Copy link
Contributor

it has a checked mark for Unit & integration tests added and passing in this PR but i cant seem to find them here.

i'm trying out this connector with one trello board ID and it's pulling in all boards and all associated streams.
(separately i did confirm that putting in an invalid board ID shows a nice error message in the UI, which is great)

@sherifnada
Copy link
Contributor

@grubberr can you create an issue to address the problem pointed out by daniel, and include unit/integration tests for this behavior so we prevent such regressions?

@harshithmullapudi
Copy link
Contributor

#8326

@erdenezul
Copy link
Author

I've made changes only in childstream which means specified board id will work in stream except the board. I've interpreted the issue not to fetch all boards every time fetch sub-streams. Sorry for missed board stream @grubberr

@danieldiamond
Copy link
Contributor

@erdenezul i have confirmed this locally. apologies, I was simply reviewed the board table to verify and should've looked at the other streams aswell. However, the expected behaviour IMO is that it should pull only data associated with the custom board IDs from all streams.
If we all agree this is expected behaviour then hopefully we can keep the newly raised issue to update the connector accordingly

@danieldiamond
Copy link
Contributor

danieldiamond commented Nov 30, 2021 •
edited
Loading

@erdenezul did you try creating a new source with this new version or use an existing configuration. I'm trying to add new API token & API key, which results in connection test failure

HTTPError('401 Client Error: Unauthorized for url: https://api.trello.com/1/members/me/boards')

I've even tried re-entering the existing ones that work and they fail too.
To confirm: I can retest my existing credentials and they work but if I type them in again, the test connection fails

possible culprit: https://github.com/airbytehq/airbyte/pull/8183/files#diff-bfa581f23992d810a07225ff1616cab03dc1df1eed6d8a93ce1aa8cf20bce8d5R205
although not sure why it works on existing credentials

@erdenezul
Copy link
Author

Screenshot 2021-11-30 at 11 58 12

@danieldiamond
Copy link
Contributor

danieldiamond commented Nov 30, 2021 •
edited
Loading

I am not sure what the above screenshot is meant to convey. i assume you have an existing working connector. can you please re-enter the same API key & token and let me know if the connector still works?

@erdenezul
Copy link
Author

@danieldiamond I've checked it with existing API with valid and invalid board id. Still works

@danieldiamond
Copy link
Contributor

@erdenezul to clarify, you have re-entered the API key & token and clicked retest?

@danieldiamond
Copy link
Contributor

omg 🤦 the fields are reversed on my config!
Screen Shot 2021-11-30 at 10 13 39 pm

@erdenezul
Copy link
Author

so you are fine now? @danieldiamond

@danieldiamond
Copy link
Contributor

on connections, yes! sorry i derailed the conversation a bit. but the original point on limiting the parent baord stream is still relevant

@erdenezul
Copy link
Author

yeah agreed

@grubberr
Copy link
Contributor

grubberr commented Nov 30, 2021 •
edited
Loading

@sherifnada working in this PR
#8338

@grubberr
Copy link
Contributor

grubberr commented Dec 1, 2021

@danieldiamond @erdenezul can you please look at this PR #8338
does it fix all known problems?

@erdenezul
Copy link
Author

Yes it will @grubberr

schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
@schlattk
Source Trello: enable specifying board ids in configuration (airbyteh…
d254791 
…q#8183)

* ⚡ Add ids of trello board in configuration airbytehq#7898

Co-authored-by: Erdenezul Batmunkh <erdenezul.batmunkh@buildingminds.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sherifnada sherifnada sherifnada left review comments

@grubberr grubberr grubberr approved these changes

@antixar antixar Awaiting requested review from antixar

Assignees

@grubberr grubberr

Labels
area/connectors Connector related issues community
Projects
No open projects
GL Roadmap
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Source Trello: allow a list of board_id in config
7 participants
@erdenezul @CLAassistant @grubberr @danieldiamond @sherifnada @harshithmullapudi @octavia-squidington-iii