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

Source google sheets: allow using spreadsheet URL instead of ID and update docs #11404

Merged
merged 5 commits into from
Mar 28, 2022

Conversation

grishick
Copy link
Contributor

@grishick grishick commented Mar 24, 2022

What

  • Update wording in spec file
  • Allow using Spreadsheet URL instead of Spreadsheet ID

How

  • add a parser helper that extracts Sheet ID from a URL
  • add a tests for parsing sheet ID out of a URL
  • add a test for backward compatibility (parser should recognize an ID)
  • change spreadsheet_id label in spec to Spreadsheet Link per github issue 11406
  • update description text for spreadsheet_id in spec

🚨 User Impact 🚨

Before this change, users had to extract spreadsheet ID from spreadsheet URL, which was causing confusion and errors. After this change, users can use the entire spreadsheet URL and copy-paste it from their Google Sheets UI directly into connector settings without having to extract the ID from the URL

Pre-merge Checklist

Expand the relevant checklist and delete the others.

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

Tests

Unit
  • added test_get_spreadsheet_id
  • added integration test with Spreadsheet URL

@grishick grishick requested a review from misteryeo March 24, 2022 17:00
@github-actions github-actions bot added the area/connectors Connector related issues label Mar 24, 2022
@codecov
Copy link

codecov bot commented Mar 24, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@787daa9). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #11404   +/-   ##
=========================================
  Coverage          ?   62.54%           
=========================================
  Files             ?        7           
  Lines             ?      307           
  Branches          ?        0           
=========================================
  Hits              ?      192           
  Misses            ?      115           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 787daa9...892a5d8. Read the comment docs.

Copy link
Contributor

@misteryeo misteryeo left a comment

Choose a reason for hiding this comment

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

Approving this as is but imagine this will change once we implement: #11406

@grishick grishick linked an issue Mar 25, 2022 that may be closed by this pull request
@grishick grishick changed the title Greg/google sheets doc update Source google sheets: allow using spreadsheet URL instead of ID and update docs Mar 25, 2022
@grishick grishick requested review from sherifnada and ChristopheDuong and removed request for sherifnada March 25, 2022 18:04
grishick added 4 commits March 25, 2022 11:10
 * Add Authentication label to credentials section
 * Add doc link to spreadsheet ID
  * add a parser helper that extracts Sheet ID from a URL
  * add a tests for parsing sheet ID out of a URL
  * add a test for backward compatibility (parser should recognize an ID)
  * change spreadsheet_id label in spec to Spreadsheet Link per github issue 11406
  * update description text for spreadsheet_id in spec
    * Replace screenshot of sheet id with screenshot of sheet URL
    * Update connector version in changelog
    * Update links to docs in spec file
@grishick grishick force-pushed the greg/google-sheets-doc-update branch from 3a3535e to 656b9d3 Compare March 25, 2022 18:25
@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Mar 25, 2022
@grishick
Copy link
Contributor Author

/test connector=connectors/source-google-sheets

2 similar comments
@sherifnada
Copy link
Contributor

/test connector=connectors/source-google-sheets

@grishick
Copy link
Contributor Author

/test connector=connectors/source-google-sheets

@grishick
Copy link
Contributor Author

grishick commented Mar 25, 2022

/test connector=connectors/source-google-sheets

🕑 connectors/source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/2042097460
❌ connectors/source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/2042097460
🐛 https://gradle.com/s/hvd47qtiuo4cw
Python short test summary info:

=========================== short test summary info ============================
FAILED test_full_refresh.py::TestFullRefresh::test_sequential_reads[inputs0]
SKIPPED [1] ../usr/local/lib/python3.7/site-packages/source_acceptance_test/plugin.py:56: Skipping TestIncremental.test_two_sequential_reads because not found in the config
============= 1 failed, 21 passed, 1 skipped in 183.54s (0:03:03) ==============

@grishick
Copy link
Contributor Author

grishick commented Mar 25, 2022

/test connector=connectors/source-google-sheets

🕑 connectors/source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/2042337978
✅ connectors/source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/2042337978
Python tests coverage:

Name                                                 Stmts   Miss  Cover
------------------------------------------------------------------------
source_acceptance_test/utils/__init__.py                 6      0   100%
source_acceptance_test/tests/__init__.py                 4      0   100%
source_acceptance_test/__init__.py                       2      0   100%
source_acceptance_test/tests/test_full_refresh.py       52      2    96%
source_acceptance_test/utils/asserts.py                 37      2    95%
source_acceptance_test/config.py                        74      6    92%
source_acceptance_test/utils/json_schema_helper.py     105     13    88%
source_acceptance_test/utils/common.py                  70     17    76%
source_acceptance_test/utils/compare.py                 62     23    63%
source_acceptance_test/tests/test_core.py              275    106    61%
source_acceptance_test/base.py                          10      4    60%
source_acceptance_test/utils/connector_runner.py       110     48    56%
source_acceptance_test/tests/test_incremental.py        69     38    45%
------------------------------------------------------------------------
TOTAL                                                  876    259    70%
Name                                                Stmts   Miss  Cover
-----------------------------------------------------------------------
google_sheets_source/models/spreadsheet_values.py      12      0   100%
google_sheets_source/models/spreadsheet.py             34      0   100%
google_sheets_source/models/__init__.py                 2      0   100%
google_sheets_source/__init__.py                        2      0   100%
google_sheets_source/helpers.py                       137     26    81%
google_sheets_source/client.py                         22      5    77%
google_sheets_source/google_sheets_source.py          103     84    18%
-----------------------------------------------------------------------
TOTAL                                                 312    115    63%

Python short test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.7/site-packages/source_acceptance_test/plugin.py:56: Skipping TestIncremental.test_two_sequential_reads because not found in the config
================== 22 passed, 1 skipped in 101.81s (0:01:41) ===================

@grishick
Copy link
Contributor Author

grishick commented Mar 25, 2022

/publish connector=connectors/source-google-sheets

🕑 connectors/source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/2042415258
❌ connectors/source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/2042415258

@grishick grishick force-pushed the greg/google-sheets-doc-update branch from 16a917a to 892a5d8 Compare March 25, 2022 22:08
@grishick
Copy link
Contributor Author

grishick commented Mar 25, 2022

/test connector=connectors/source-google-sheets

🕑 connectors/source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/2042474535
✅ connectors/source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/2042474535
Python tests coverage:

Name                                                 Stmts   Miss  Cover
------------------------------------------------------------------------
source_acceptance_test/utils/__init__.py                 6      0   100%
source_acceptance_test/tests/__init__.py                 4      0   100%
source_acceptance_test/__init__.py                       2      0   100%
source_acceptance_test/tests/test_full_refresh.py       52      2    96%
source_acceptance_test/utils/asserts.py                 37      2    95%
source_acceptance_test/config.py                        74      6    92%
source_acceptance_test/utils/json_schema_helper.py     105     13    88%
source_acceptance_test/utils/common.py                  70     17    76%
source_acceptance_test/utils/compare.py                 62     23    63%
source_acceptance_test/tests/test_core.py              275    106    61%
source_acceptance_test/base.py                          10      4    60%
source_acceptance_test/utils/connector_runner.py       110     48    56%
source_acceptance_test/tests/test_incremental.py        69     38    45%
------------------------------------------------------------------------
TOTAL                                                  876    259    70%
Name                                                Stmts   Miss  Cover
-----------------------------------------------------------------------
google_sheets_source/models/spreadsheet_values.py      12      0   100%
google_sheets_source/models/spreadsheet.py             34      0   100%
google_sheets_source/models/__init__.py                 2      0   100%
google_sheets_source/__init__.py                        2      0   100%
google_sheets_source/helpers.py                       137     26    81%
google_sheets_source/client.py                         22      5    77%
google_sheets_source/google_sheets_source.py          103     84    18%
-----------------------------------------------------------------------
TOTAL                                                 312    115    63%

Python short test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.7/site-packages/source_acceptance_test/plugin.py:56: Skipping TestIncremental.test_two_sequential_reads because not found in the config
================== 22 passed, 1 skipped in 124.44s (0:02:04) ===================

@grishick
Copy link
Contributor Author

grishick commented Mar 25, 2022

/publish connector=connectors/source-google-sheets

🕑 connectors/source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/2042521025
✅ connectors/source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/2042521025

@grishick grishick merged commit 1d12c7c into master Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source Google Sheets: Update spreadsheet ID field for usability
3 participants