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

Add support for Smartsheet Reports (full refresh only) #46379

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add support for Smartsheet Reports (full refresh only) #46379

wants to merge 5 commits into from

Conversation

bffrost
Copy link

@bffrost bffrost commented Oct 4, 2024

What

Adds support for reading from Smartsheet Reports. This addresses #35197. Because Reports are derived views on data in Smartsheet, they don't seem to support incremental querying via the API, so only a full refresh strategy is implemented here.

How

This adds an "Is Report" config that tells the source whether the provided sheet ID is actually a sheet or a report. The connector then implements slightly different behavior for how to read from a Report vs the existing logic that only works with Sheets.

Review guide

  • See new config is_report added to spec.json
  • Refactored common logic in streams.py to add a SmartsheetReportStream which does not have incremental syncs
  • Modified read behavior in _fetch_sheet to get the entire report rather than one row at a time
  • Implemented support for "virtual columns" which show up in Reports and were breaking the existing _construct_record logic

User Impact

For existing connections this should not change the behavior at all. It will allow users to configure Smartsheet Reports as sources, so it will unlock additional ways to connect data.

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

@CLAassistant
Copy link

CLAassistant commented Oct 4, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@octavia-squidington-iii octavia-squidington-iii added the area/connectors Connector related issues label Oct 4, 2024
@bffrost bffrost temporarily deployed to community-ci-auto October 4, 2024 15:36 — with GitHub Actions Inactive
@vercel Vercel
Copy link

vercel bot commented Oct 4, 2024

@bffrost is attempting to deploy a commit to the Airbyte Growth Team on Vercel.

A member of the Team first needs to authorize it.

bffrost
bffrost commented Oct 4, 2024
View reviewed changes
airbyte-integrations/connectors/source-smartsheets/source_smartsheets/sheet.py
@@ -120,7 +164,7 @@ def json_schema(self) -> Dict[str, Any]:
}
return json_schema

def read_records(self, from_dt: str) -> Iterable[Dict[str, str]]:
def read_records(self, from_dt: Optional[str] = None) -> Iterable[Dict[str, str]]:
Copy link
Author

Choose a reason for hiding this comment

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

This argument was already optional within self._fetch_sheet

@bffrost bffrost temporarily deployed to community-ci-auto October 4, 2024 15:52 — with GitHub Actions Inactive
@bffrost bffrost temporarily deployed to community-ci-auto October 4, 2024 15:52 — with GitHub Actions Inactive
@natikgadzhi
Copy link
Contributor

natikgadzhi commented Oct 5, 2024 •
edited by github-actions bot
Loading

/bump-version type="minor" changelog="Add reports support"

Bump Version job started... Check job output.

🔴 Job completed successfully (no changes, this is sus).

natikgadzhi
natikgadzhi reviewed Oct 5, 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.

Thank you for this!

The code looks good. I'm trying to bump the version automatically, let's see if that will work. After that, I'll kick-off full CI and see how this works.

airbyte-integrations/connectors/source-smartsheets/source_smartsheets/spec.json
"title": "Is Report",
"type": "boolean",
"description": "If true, the source will treat the provided sheet_id as a report. If false, the source will treat the provided sheet_id as a sheet.",
"default": false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice touch to have the default value.

@bffrost
Copy link
Author

bffrost commented Oct 28, 2024

Thank you for this!

The code looks good. I'm trying to bump the version automatically, let's see if that will work. After that, I'll kick-off full CI and see how this works.

@natikgadzhi it looks like the automatic version bumped failed - possibly something to do with the way I forked the airbyte repo?
https://github.com/airbytehq/airbyte/actions/runs/11190256225/job/31111735437#step:5:381

23: exec git fetch target bffrost/smartsheet-report-35197 ERROR: process "git fetch target bffrost/smartsheet-report-35197" did not complete successfully: exit code: 128
23: [0.19s] fatal: couldn't find remote ref bffrost/smartsheet-report-35197

I'm not sure how to address this but please let me know if there's anything I can do to get this fixed up and mergeable

@natikgadzhi
Copy link
Contributor

@bffrost wooops. Sorry for the delay, got into company offsite and then one thing after the other.

You can bump versions manually.

  • In pyproject.toml file
  • In metadata.yaml file (dockerImageTag)
  • Add a changelog line in docs/integrations/sources/smartsheets.md

@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Nov 14, 2024
@bffrost bffrost temporarily deployed to community-ci-auto November 14, 2024 16:33 — with GitHub Actions Inactive
@bffrost bffrost temporarily deployed to community-ci-auto November 14, 2024 16:33 — with GitHub Actions Inactive
@bffrost
Copy link
Author

bffrost commented Nov 14, 2024

@natikgadzhi My turn for a delayed response 😅
I've done the manual version bump & updated this branch with latest changes from master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@natikgadzhi natikgadzhi natikgadzhi left review comments

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.
4 participants
@bffrost @CLAassistant @natikgadzhi @octavia-squidington-iii