🐛 Source Github: Fix set up validation #5456

yevhenii-ldv · 2021-08-17T07:18:46Z

What

closes #5434.

How

Describe the solution

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

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

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
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions
Connector version bumped like described here

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

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.

yevhenii-ldv · 2021-08-17T07:26:03Z

/test connector=connectors/source-github

🕑 connectors/source-github https://github.com/airbytehq/airbyte/actions/runs/1138365066
❌ connectors/source-github https://github.com/airbytehq/airbyte/actions/runs/1138365066

yevhenii-ldv · 2021-08-17T07:52:22Z

/test connector=connectors/source-github

🕑 connectors/source-github https://github.com/airbytehq/airbyte/actions/runs/1138431723
✅ connectors/source-github https://github.com/airbytehq/airbyte/actions/runs/1138431723

marcosmarxm · 2021-08-17T12:08:29Z

airbyte-integrations/connectors/source-github/source_github/source.py

+        organizations = list(filter(None, config.get("organization", "").split(" ")))
+        repositories = list(filter(None, config.get("repository", "").split(" ")))


If user doesn't send the two field any change to print the exception? Because both will become a empty string and not a None value.

@marcosmarxm Sorry, I didn't quite understand what you meant, could you reformulate it?

@yevhenii-ldv we don't handle the case when both fields are empty. I think we should either handle this by schema (use oneOf) or in the code.

also about repo/org usage, not sure which one will take precedence. Maybe it is better to use a single field and ask a user to use format:
<org>:<repo> or simply <org> (or <org>:*)
i.e.

repositories = "airbyte:*,eugene:airbyte"

@keu @marcosmarxm
Unfortunately, in order to maintain backward compatibility, the Sheriff and I made a decision that using oneOf will not work in this case and it is necessary to check directly in the check method, and not in the spec.json.

@keu As for Organizations and Repositories - now there have absolutely equal rights and we receive data for absolutely all repositories that we received using both fields.

please use single field repositories with the following format acceptable:

org/repo - sync concrete repo

org/ - sync all repos from org

marcosmarxm · 2021-08-17T12:11:10Z

airbyte-integrations/connectors/source-github/source_github/streams.py

@@ -127,6 +127,8 @@ def read_records(self, stream_slice: Mapping[str, any] = None, **kwargs) -> Iter
                # For private repositories `Teams` stream is not available and we get "404 Client Error: Not Found for
                # url: https://api.github.com/orgs/sherifnada/teams?per_page=100" error.
                error_msg = f"Syncing `Team` stream isn't available for repository `{stream_slice['repository']}`."
+            else:
+                raise e


Can we have an additional information where this error is being thrown? Maybe

else: logging.ERROR("Undefined error while reading records") raise e

yevhenii-ldv · 2021-08-17T18:16:17Z

/test connector=connectors/source-github

🕑 connectors/source-github https://github.com/airbytehq/airbyte/actions/runs/1140364264
❌ connectors/source-github https://github.com/airbytehq/airbyte/actions/runs/1140364264

yevhenii-ldv · 2021-08-17T18:40:54Z

/test connector=connectors/source-github

🕑 connectors/source-github https://github.com/airbytehq/airbyte/actions/runs/1140434490
✅ connectors/source-github https://github.com/airbytehq/airbyte/actions/runs/1140434490

marcosmarxm · 2021-08-17T18:56:51Z

airbyte-integrations/connectors/source-github/source_github/source.py


-        repositories_list = []
+        repositories_list = [repo for repo in repositories if not re.match("^.*/\\*$", repo)]
+        organizations = [org.split("/")[0] for org in repositories if org not in repositories_list]


Are we going to pull all repos from organization without the user request this? @yevhenii-ldv

@marcosmarxm If the user specifies, for example, airbytehq/*, this means that he wants to pull all the repositories for this organization.

btw, there is no unit_test for this? would be amazing to have a test mocking this function.

marcosmarxm

LGTM, but this private functions must have functions

yevhenii-ldv · 2021-08-18T12:09:41Z

/publish connector=connectors/source-github

🕑 connectors/source-github https://github.com/airbytehq/airbyte/actions/runs/1143046034
❌ connectors/source-github https://github.com/airbytehq/airbyte/actions/runs/1143046034

yevhenii-ldv · 2021-08-18T12:21:40Z

/publish connector=connectors/source-github

🕑 connectors/source-github https://github.com/airbytehq/airbyte/actions/runs/1143084463
✅ connectors/source-github https://github.com/airbytehq/airbyte/actions/runs/1143084463

Fix set up validation

f42c2d2

yevhenii-ldv changed the title ~~bug Source Github: Fix set up validation~~ Source Github: Fix set up validation Aug 17, 2021

github-actions bot added the area/connectors Connector related issues label Aug 17, 2021

yevhenii-ldv changed the title ~~Source Github: Fix set up validation~~ 🐛 Source Github: Fix set up validation Aug 17, 2021

yevhenii-ldv requested a review from keu August 17, 2021 07:19

jrhizor temporarily deployed to more-secrets August 17, 2021 07:28 Inactive

raise exception

62a46b2

jrhizor temporarily deployed to more-secrets August 17, 2021 07:54 Inactive

marcosmarxm requested changes Aug 17, 2021

View reviewed changes

add log for error

2e671e6

yevhenii-ldv requested a review from marcosmarxm August 17, 2021 14:19

update code

bbe3cbb

github-actions bot added the area/documentation Improvements or additions to documentation label Aug 17, 2021

jrhizor temporarily deployed to more-secrets August 17, 2021 18:18 Inactive

jrhizor temporarily deployed to more-secrets August 17, 2021 18:30 Inactive

jrhizor temporarily deployed to more-secrets August 17, 2021 18:43 Inactive

marcosmarxm requested changes Aug 17, 2021

View reviewed changes

yevhenii-ldv requested a review from marcosmarxm August 17, 2021 19:50

marcosmarxm approved these changes Aug 17, 2021

View reviewed changes

keu approved these changes Aug 18, 2021

View reviewed changes

update Changelogs and bump version

8012391

jrhizor temporarily deployed to more-secrets August 18, 2021 12:12 Inactive

jrhizor temporarily deployed to more-secrets August 18, 2021 12:23 Inactive

yevhenii-ldv merged commit 03e1ff9 into master Aug 18, 2021

yevhenii-ldv deleted the ykurochkin/github-connector-set-up-validation-failing-on-non-required-field branch August 18, 2021 13:03

igrankova added connectors/sources-api connectors/source/github labels Jan 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Source Github: Fix set up validation #5456

🐛 Source Github: Fix set up validation #5456

yevhenii-ldv commented Aug 17, 2021

yevhenii-ldv commented Aug 17, 2021 •

edited by github-actions bot

Loading

yevhenii-ldv commented Aug 17, 2021 •

edited by github-actions bot

Loading

marcosmarxm Aug 17, 2021

yevhenii-ldv Aug 17, 2021

keu Aug 17, 2021

keu Aug 17, 2021 •

edited

Loading

yevhenii-ldv Aug 17, 2021

yevhenii-ldv Aug 17, 2021

keu Aug 17, 2021

yevhenii-ldv Aug 17, 2021

marcosmarxm Aug 17, 2021

yevhenii-ldv Aug 17, 2021

yevhenii-ldv commented Aug 17, 2021 •

edited by github-actions bot

Loading

yevhenii-ldv commented Aug 17, 2021 •

edited by github-actions bot

Loading

marcosmarxm Aug 17, 2021

yevhenii-ldv Aug 17, 2021

marcosmarxm Aug 17, 2021

marcosmarxm left a comment

yevhenii-ldv commented Aug 18, 2021 •

edited by github-actions bot

Loading

yevhenii-ldv commented Aug 18, 2021 •

edited by github-actions bot

Loading

		organizations = list(filter(None, config.get("organization", "").split(" ")))
		repositories = list(filter(None, config.get("repository", "").split(" ")))

🐛 Source Github: Fix set up validation #5456

🐛 Source Github: Fix set up validation #5456

Conversation

yevhenii-ldv commented Aug 17, 2021

What

How

Recommended reading order

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

yevhenii-ldv commented Aug 17, 2021 • edited by github-actions bot Loading

yevhenii-ldv commented Aug 17, 2021 • edited by github-actions bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keu Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yevhenii-ldv commented Aug 17, 2021 • edited by github-actions bot Loading

yevhenii-ldv commented Aug 17, 2021 • edited by github-actions bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcosmarxm left a comment

Choose a reason for hiding this comment

yevhenii-ldv commented Aug 18, 2021 • edited by github-actions bot Loading

yevhenii-ldv commented Aug 18, 2021 • edited by github-actions bot Loading

yevhenii-ldv commented Aug 17, 2021 •

edited by github-actions bot

Loading

yevhenii-ldv commented Aug 17, 2021 •

edited by github-actions bot

Loading

keu Aug 17, 2021 •

edited

Loading

yevhenii-ldv commented Aug 17, 2021 •

edited by github-actions bot

Loading

yevhenii-ldv commented Aug 17, 2021 •

edited by github-actions bot

Loading

yevhenii-ldv commented Aug 18, 2021 •

edited by github-actions bot

Loading

yevhenii-ldv commented Aug 18, 2021 •

edited by github-actions bot

Loading