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 Github: better error explanation for 401 #26025

Merged

Conversation

roman-yermilov-gl
Copy link
Collaborator

@roman-yermilov-gl roman-yermilov-gl commented May 12, 2023

What

Not every case handled when 401 exception happen. Unhandled 401 error gives traceback instead of human readable error description.

How

*Added log with more transparent depiction of the personal access token expired instead of just getting error traceback. For this I made the following changes:

  • Pass the authentication type variable to constructor method of each stream because we need to know token of what type we currently deal with (Accordion to task description we must handle personal access token only but there one more type)
  • Add condition to check whether error code 401 or not and also check the token type. If all is true then log a message: <token type> renewal is required: <original error message>. Of course I raise original error in the end to do not break the flow
  • Fix acceptance test config: there was expected records changed so I ignored fast changing field
  • Fix unit test: because of changes in step 1 the _get_authenticator method become not static, so SourceGithub need to be instantiated now to call it
  • Add unit test to ensure 401 error is properly handled
  • Make refactoring to avoid code duplicates: move constants from source and stream modules to separate module as we use it in two different places now*

@github-actions
Copy link
Contributor

github-actions bot commented May 12, 2023

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

  • PR name follows PR naming conventions
  • Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan and you've followed all steps in the Breaking Changes Checklist
  • Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
  • Secrets in the connector's spec are annotated with airbyte_secret
  • All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
  • Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
  • You, or an Airbyter, have run /test successfully on this PR - or on a non-forked branch
  • You, or an Airbyter, have run /publish successfully on this PR - or on a non-forked branch
  • You've updated the connector's metadata.yaml file new!

If the checklist is complete, but the CI check is failing,

  1. Check for hidden checklists in your PR description

  2. Toggle the github label checklist-action-run on/off to re-run the checklist CI.

@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label May 12, 2023
@roman-yermilov-gl
Copy link
Collaborator Author

roman-yermilov-gl commented May 12, 2023

/test connector=connectors/source-github

🕑 connectors/source-github https://github.com/airbytehq/airbyte/actions/runs/4958689192
❌ connectors/source-github https://github.com/airbytehq/airbyte/actions/runs/4958689192
🐛 https://gradle.com/s/6cgg3wfy3yzy2

Build Failed

Test summary info:

Could not find result summary

@roman-yermilov-gl
Copy link
Collaborator Author

roman-yermilov-gl commented May 12, 2023

/test connector=connectors/source-github

🕑 connectors/source-github https://github.com/airbytehq/airbyte/actions/runs/4959377642
❌ connectors/source-github https://github.com/airbytehq/airbyte/actions/runs/4959377642
🐛 https://gradle.com/s/2kdch25etbryq

Build Failed

Test summary info:

	 =========================== short test summary info ============================
	 FAILED unit_tests/test_stream.py::test_stream_workflow_runs_read_incremental
	 FAILED unit_tests/unit_test.py::test_single_token - TypeError: _get_authentic...
	 FAILED unit_tests/unit_test.py::test_multiple_tokens - TypeError: _get_authen...
	 �[31m================== �[31m�[1m3 failed�[0m, �[32m54 passed�[0m, �[33m146 warnings�[0m�[31m in 2.11s�[0m�[31m ==================�[0m

@roman-yermilov-gl
Copy link
Collaborator Author

roman-yermilov-gl commented May 15, 2023

/test connector=connectors/source-github

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

Name                             Stmts   Miss  Cover
----------------------------------------------------
source_github/github_schema.py    8807      0   100%
source_github/constants.py           5      0   100%
source_github/__init__.py            2      0   100%
source_github/utils.py              16      1    94%
source_github/streams.py           845     91    89%
source_github/graphql.py           167     22    87%
source_github/source.py            140     37    74%
----------------------------------------------------
TOTAL                             9982    151    98%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [2] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:578: The previous and actual discovered catalogs are identical.
================== 47 passed, 3 skipped in 314.53s (0:05:14) ===================

Copy link
Collaborator

@lazebnyi lazebnyi left a comment

Choose a reason for hiding this comment

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

Please update description to what and how master will be changed with your PR.

@roman-yermilov-gl roman-yermilov-gl force-pushed the ryermilov/source-github-pat-401-error-message branch from 4418584 to 6495726 Compare May 16, 2023 13:37
@roman-yermilov-gl
Copy link
Collaborator Author

roman-yermilov-gl commented May 16, 2023

/publish connector=connectors/source-github

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


Connector Version Did it publish? Were definitions generated?
connectors/source-github 0.4.10

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

@roman-yermilov-gl roman-yermilov-gl force-pushed the ryermilov/source-github-pat-401-error-message branch from 876fd9d to 43dbbd5 Compare May 16, 2023 16:18
@roman-yermilov-gl
Copy link
Collaborator Author

roman-yermilov-gl commented May 16, 2023

/publish connector=connectors/source-github

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


Connector Version Did it publish? Were definitions generated?
connectors/source-github 0.4.11

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

@roman-yermilov-gl roman-yermilov-gl merged commit ce9eb8a into master May 16, 2023
30 of 31 checks passed
@roman-yermilov-gl roman-yermilov-gl deleted the ryermilov/source-github-pat-401-error-message branch May 16, 2023 18:35
marcosmarxm pushed a commit to natalia-miinto/airbyte that referenced this pull request Jun 8, 2023
* Source Github: better error explanation for 401

* Source Github: bump version, update changelog

* Source Github: fix test name

* Source Github: fix unittests

* Source Github: bump version

* auto-bump connector version

---------

Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
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 connectors/source/github
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source Github: update personal access token expired error
3 participants