Skip to content

Fix pagination in Aircall connector #44437

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
Merged
merged 9 commits into from
Aug 20, 2024
Merged

Fix pagination in Aircall connector #44437

merged 9 commits into from
Aug 20, 2024

Conversation

sidoh
Copy link
Contributor

@sidoh sidoh commented Aug 20, 2024

What

The Aircall connector defined paginator under record_selector, which wasn't getting picked up. This corrects the indentation. Think this probably explains #42092.

Based on the documentation, I think the pagination logic was also slightly off -- went ahead and fixed that as well.

How

Sets indent to correct level.

Use "page" for parameter instead of a jinja template.

Review guide

User Impact

Shouldn't be anything negative. Expect this connector wasn't working for any streams that needed pagination.

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

@vercel Vercel
Copy link

vercel bot commented Aug 20, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 20, 2024 7:48pm

@sidoh sidoh temporarily deployed to community-ci-auto August 20, 2024 00:11 — with GitHub Actions Inactive
@sidoh sidoh temporarily deployed to community-ci-auto August 20, 2024 00:11 — with GitHub Actions Inactive
natikgadzhi
natikgadzhi approved these changes Aug 20, 2024
View reviewed changes
Copy link
Contributor

@natikgadzhi natikgadzhi left a comment

Choose a reason for hiding this comment

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

Approving caveated that CI should be happy — this will need a version bump

airbyte-integrations/connectors/source-aircall/metadata.yaml
@@ -2,7 +2,7 @@ data:
connectorSubtype: api
connectorType: source
definitionId: 912eb6b7-a893-4a5b-b1c0-36ebbe2de8cd
dockerImageTag: 0.2.12
dockerImageTag: 0.3.0
Copy link
Contributor

Choose a reason for hiding this comment

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

It would demand that you also add a change entry to docs/integrations/aircall.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

airbyte-integrations/connectors/source-aircall/source_aircall/manifest.yaml Outdated
page_size_option:
type: "RequestOption"
inject_into: "request_parameter"
field_name: "{{ response['meta']['per_page'] }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This still looks sus — shouldn't field_name be just per_page or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh shoot, this is the same thing I fixed for page_token_option haha. Yeah that's right, I'll fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed! ty

@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Aug 20, 2024
@sidoh sidoh temporarily deployed to community-ci-auto August 20, 2024 00:25 — with GitHub Actions Inactive
@sidoh sidoh temporarily deployed to community-ci-auto August 20, 2024 00:29 — with GitHub Actions Inactive
@sidoh sidoh temporarily deployed to community-ci-auto August 20, 2024 00:29 — with GitHub Actions Inactive
@sidoh sidoh enabled auto-merge (squash) August 20, 2024 01:21
@sidoh
Copy link
Contributor Author

sidoh commented Aug 20, 2024

thanks for the fast review, @natikgadzhi! seems like some of the checks need some kind of additional approval? not familiar with the process. lmk if i'm missing anything.

@natikgadzhi
Copy link
Contributor

It wants this:

source-aircall - ❌ Failed - Connector version in metadata.yaml and pyproject.toml file must match: Version is 0.3.0 in metadata.yaml, but version is 0.2.12 in pyproject.toml. These two files have to be consistent.
Error: 1 checks failed

Sorry, things are verbose ;(

@sidoh
Copy link
Contributor Author

sidoh commented Aug 20, 2024

It wants this:

source-aircall - ❌ Failed - Connector version in metadata.yaml and pyproject.toml file must match: Version is 0.3.0 in metadata.yaml, but version is 0.2.12 in pyproject.toml. These two files have to be consistent.
Error: 1 checks failed

Sorry, things are verbose ;(

Updated!

@sidoh sidoh temporarily deployed to community-ci-auto August 20, 2024 19:43 — with GitHub Actions Inactive
@sidoh sidoh temporarily deployed to community-ci-auto August 20, 2024 19:43 — with GitHub Actions Inactive
@sidoh sidoh merged commit 2b1f42f into airbytehq:master Aug 20, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@natikgadzhi natikgadzhi natikgadzhi approved these changes

Assignees
No one assigned
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/aircall
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sidoh @natikgadzhi @octavia-squidington-iii